ARM-software / trappy

This repository has moved to https://gitlab.arm.com/tooling/trappy
Apache License 2.0
60 stars 39 forks source link

Urgent: Fix parsing when a bad trace.txt exists #267

Closed bjackman closed 6 years ago

bjackman commented 6 years ago

TRAPpy requires that trace.txt files are created with certain events printed raw, rather than formated. When given a trace.dat, it currently creates a trace.txt that fits that requirement, then uses that for parsing.

It used to create one trace.txt with non-raw formatting, and a trace.raw.txt with raw formatting. Therefore it currently uses the presence of trace.raw.txt as a hint to suggest that the trace.txt contains non-raw formatting (as created by an older version of TRAPpy), and raises an error in this case.

However bad (non-raw) trace.txt files may have been created by the user or other tools such as Workload Automation[1].

Therefore, this patch presents a new approach to finding and creating input files:

[1] https://github.com/ARM-software/workload-automation

valschneider commented 6 years ago

Trappy isn't my turf but the code LGTM. Update logic is sound to me too, I just have a question regarding the trace.txt handling.

IIUC, Trappy will now read a trace.dat file and create a trace.trappy.txt file for parsing. If trace.dat is not here but trace.trappy.txt is, warn and continue - so far so good. However, I don't get where the trace.txt is coming from. Is that just legacy support ? We're not supposed to generate any trace.txt file anymore, right ?

bjackman commented 6 years ago

However, I don't get where the trace.txt is coming from. Is that just legacy support ? We're not supposed to generate any trace.txt file anymore, right ?

Yeah, legacy support and so that users who know what they're doing and want to create textual traces (using the correct trace-cmd args) can still use them.

valschneider commented 6 years ago

Yeah, legacy support and so that users who know what they're doing and want to create textual traces (using the correct trace-cmd args) can still use them.

Okay, it all LGTM then.

bjackman commented 6 years ago

This is superseded by the superior https://github.com/ARM-software/trappy/pull/270, because that PR means we stop parsing trace.txt unless it's the only thing we have.