Re: [PATCH] perf scripts python arm-cs-trace-disasm.py: Skip disasm if address continuity is broken

[James Clark]

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.