search

andikleen / pmu-tools

Intel PMU profiling tools
GNU General Public License v2.0
1.99k stars 332 forks source link

Event Parser Fix #397

Closed matthew-olson-intel closed 2 years ago

matthew-olson-intel commented 2 years ago

Sorry this PR adds so much more complexity to the parser; I had to steadily add checks to make it more robust.

andikleen commented 2 years ago 
    memmove(event, event + 1, 199);

Surely strlen(event+1)+1 not 199?

Also please place the brackets in the same way as the rest of the code.

andikleen commented 2 years ago

Also it would be nice if you could add test cases for the new code to jevents/tester

matthew-olson-intel commented 2 years ago

ACK-- hard to overcome muscle memory.

Yes, you're right, using strlen there is definitely better: I just hardcoded 199 because that line was originally within the jevent_name_to_attr_extra function, which uses a 200-byte buffer.

matthew-olson-intel commented 2 years ago

OK, fixed a double-free error when failing to resolve an event: that's not related to the changes, though.

Didn't notice that the tester was disabled; I thought that I'd passed all the tests. Running it yields an error:

Unknown modifier r at end for cpu/umask=0x01,period=100003,event=0xb7,offcore_rsp=0x80400122/request=all_data_rd,response=any_response

It looks like that string doesn't work with perf, either, though. Think it should be changed in the tester, or changed in the parser?

andikleen commented 2 years ago

Looks ok to me now, but there' s a conflict now according to github.

Can you rebase and push again? During the rebase please also squash the style fixes into the original patches.

matthew-olson-intel commented 2 years ago

Rebased, squashed all style-fixing commits, and merged upstream changes back in. Should be good now!

andikleen commented 2 years ago

Github still didn't like it, but I merged it manually now. Thanks!