Re: [Qemu-devel] [PATCH] trace: fix group name generation

[Greg Kurz]

On Tue, 18 Oct 2016 16:31:24 +0100
"Daniel P. Berrange" <address@hidden> wrote:

> On Tue, Oct 18, 2016 at 04:16:46PM +0100, Daniel P. Berrange wrote:
> > On Fri, Oct 14, 2016 at 04:31:01PM -0500, Eric Blake wrote:  
> > > On 10/14/2016 04:26 PM, Greg Kurz wrote:  
> > > > Since commit "80dd5c4918ab trace: introduce a formal group name for 
> > > > trace
> > > > events", tracetool generates C variable names and macro definitions out
> > > > of the path to the trace-events-all file.
> > > > 
> > > > The current code takes care of turning '/' and '-' characters into
> > > > underscores so that the resulting names are valid C tokens. This is
> > > > enough because these are the only illegal characters that appear in
> > > > a relative path within the QEMU source tree.
> > > > 
> > > > Things are different for out of tree builds where the path may contain
> > > > arbitrary character combinations, causing tracetool to generate invalid
> > > > names.
> > > >   
> > >   
> > > > This patch ensures that only letters [A-Za-z], digits [0-9] and 
> > > > underscores
> > > > are kept. All other characters are turned into underscores. Also, since 
> > > > the
> > > > first character of C symbol names cannot be a digit, an underscore is
> > > > prepended to the group name.
> > > > 
> > > > Signed-off-by: Greg Kurz <address@hidden>
> > > > ---
> > > >  scripts/tracetool.py |    2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/scripts/tracetool.py b/scripts/tracetool.py
> > > > index 629b2593c846..b81b834db924 100755
> > > > --- a/scripts/tracetool.py
> > > > +++ b/scripts/tracetool.py
> > > > @@ -70,7 +70,7 @@ def make_group_name(filename):
> > > >  
> > > >      if dirname == "":
> > > >          return "common"
> > > > -    return re.sub(r"/|-", "_", dirname)
> > > > +    return "_" + re.sub(r"[^\w]", "_", dirname)  
> > > 
> > > This STILL doesn't solve the complaint that the build is now dependent
> > > on the location.  Why can't we STRIP off any prefix prior to the in-tree
> > > portion of the naming that we know is sane, instead of munging the
> > > prefix but in the process creating source code that generates with
> > > different lengths?
> > > 
> > > Ideally, compiling twice, once in directory 'a', and the second time in
> > > directory 'aaaaaaaaaaaaaaaaaaaaaaaaaaaa', should not make a noticeable
> > > difference in the final size of the executable due to the difference in
> > > lengths of the debugging symbols used to record the longer name of the
> > > second directory being encoded into lots of macro names.  
> > 
> > This is a mistake on my part - the code was supposed to be stripping
> > off the build directory prefix, leaving only the relative path to the
> > file wrt source directory. The code is simply wrong as is.  
> 
> Ah ha, I realize what the issue is.
> 
> Currently in git master we have multiple trace-events files and we merge
> them into a single trace-events-all file, then generate the various
> bits we need. This trace-events-all file is naturally in the build
> dir, not the source dir
> 
> In my trace events patch build refactor series though, I have stopped
> creating trace-events-all, and we instead generate bits directly from
> the trace-events files in source dir. So this problem only appeared
> because we've only merge part of my series into master.
> 

Heh commit 80dd5c4918ab then makes sense in this scenario, except perhaps
the nit about --group in the changelog.

> IOW, I think Greg's proposed fix is fine as a workaround - once the
> rest of my patches merge, build dir should not pollute this at all.
> 

I have two other patches ready to fix the current situation:
- one using os.getcwd() to guess the build directory
- one implementing --group as mentioned in my other mail

But the one that filters unwanted characters is a less intrusive
workaround.

> Regards,
> Daniel

Cheers.

---
Greg