On Thu, 8 Aug 2024 at 12:15, Ganapatrao Kulkarni gankulkarni@os.amperecomputing.com wrote:

On 08-08-2024 04:21 pm, James Clark wrote:

...

On 08/08/2024 10:21 am, James Clark wrote:

...

On 08/08/2024 8:42 am, Leo Yan wrote:

...

On 8/8/2024 5:36 AM, Ganapatrao Kulkarni wrote:

...

On 08-08-2024 12:50 am, Leo Yan wrote:

...

On 8/7/2024 5:18 PM, Ganapatrao Kulkarni wrote:

> Is below diff with force option looks good? > > diff --git a/tools/perf/scripts/python/arm-cs-trace-disasm.py > b/tools/perf/scripts/python/arm-cs-trace-disasm.py > index d973c2baed1c..efe34f308beb 100755 > --- a/tools/perf/scripts/python/arm-cs-trace-disasm.py > +++ b/tools/perf/scripts/python/arm-cs-trace-disasm.py > @@ -36,7 +36,10 @@ option_list = [ > help="Set path to objdump executable file"), > make_option("-v", "--verbose", dest="verbose", > action="store_true", default=False, > - help="Enable debugging log") > + help="Enable debugging log"), > + make_option("-f", "--force", dest="force", > + action="store_true", default=False, > + help="Force decoder to continue") > ] > > parser = OptionParser(option_list=option_list) > @@ -257,6 +260,12 @@ def process_event(param_dict): > print("Stop address 0x%x is out of range [ 0x%x > .. 0x%x > ] for dso %s" % (stop_addr, int(dso_start), int(dso_end), dso)) > return > > + if (stop_addr < start_addr): > + if (options.verbose == True or options.force): > + print("Packet Discontinuity detected > [stop_add:0x%x start_addr:0x%x ] for dso %s" % (stop_addr, > start_addr, dso))

The options.force for the print should be "options.verbose or not options.force" I think? You want to print the error until the user adds -f, then hide it. Unless verbose is on.

...

...

...

> + if (options.force): > + return

Oops I had this one the wrong way around in my example. This way is correct.

...

...

...

I struggled a bit for the code - it is confused that force mode bails out and the non-force mode continues to run. I prefer to always bail out for the discontinuity case, as it is pointless to continue in this case.

Kept bail out with force option since I though it is not good to hide the error in normal use, otherwise we never able to notice this error in the future and it becomes default hidden. Eventually this error should be fixed.

As James said, the issue should be fixed in OpenCSD or Perf decoding flow.

Thus, perf tool should be tolerant errors - report warning and drop discontinuous samples. This would be easier for developers later if face the same issue, they don't need to spend time to locate issue and struggle for overriding the error.

If you prefer to use force option, it might be better to give reasoning and *suggestion* in one go, something like:

 if (stop_addr < start_addr):
    print("Packet Discontinuity detected [stop_add:0x%x

start_addr:0x%x ] for dso %s" % (stop_addr, start_addr, dso)) print("Use option '-f' following the script for force mode" if (options.force) return

Either way is fine for me. Thanks a lot for taking time on the issue.

Leo

But your diff looks good Ganapat, I think send a patch with Leo's extra help message added and the first force flipped.

One other small detail about Leo's suggestion print out. Can you add an instruction of how to keep the warnings as well:

print("Use option '-f' following the script for force mode. Add '-v' \ to continue printing decode warnings.")

Thanks James and Leo for your comments. I will send the V2 with the changes as discussed.

Thanks. Ganapat

Certainly any ARM trace decode is dependent on accurate program images being input to provide correct trace decode at the output.

So if an OpenCSD client does not provide accurate information then it should not really expect to get accurate trace as an output!

That said there are certainly a couple of changes that can be made:-

1) Clearly outputting a trace range with a finish address lower than the start address is incorrect. This can unconditionally output a hard error.

2) Detection of non-cond not taken should be added as a configurable option. - either all, or direct only. This can be achieved by adding flags to the library configuration API. For a client like perf - these could be controlled by the verbose level - which I believe is an int in the range 0-10 or something?

However - when we do detect these errors, it is essential that the entire decoder is reset and tracing not restarted till the next sync point in the trace data. i.e. assuming that the next range that happens to be consecutive after a break, given a prior input of incorrect address data is simply invalid. There is no way of knowing if the branch taken / not taken sequence matches the addresses in the program image any more.

The solution that James proposes above, needs to actually generate an error which will then automatically reset the decoder to an unsynced state.

Regards

Mike

-- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK