From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41570) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bwWgW-0007YG-T1 for qemu-devel@nongnu.org; Tue, 18 Oct 2016 11:52:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bwWgR-0002c9-T7 for qemu-devel@nongnu.org; Tue, 18 Oct 2016 11:52:12 -0400 Received: from 2.mo179.mail-out.ovh.net ([178.33.250.45]:33866) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bwWgR-0002bh-N5 for qemu-devel@nongnu.org; Tue, 18 Oct 2016 11:52:07 -0400 Received: from player792.ha.ovh.net (b7.ovh.net [213.186.33.57]) by mo179.mail-out.ovh.net (Postfix) with ESMTP id 5587A3B11 for ; Tue, 18 Oct 2016 17:52:06 +0200 (CEST) Date: Tue, 18 Oct 2016 17:52:00 +0200 From: Greg Kurz Message-ID: <20161018175200.48a2d3cc@bahia> In-Reply-To: <20161018153124.GB15199@redhat.com> References: <147648037766.14770.15179642645810274907.stgit@bahia> <20161018151646.GI12728@redhat.com> <20161018153124.GB15199@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] trace: fix group name generation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" Cc: Eric Blake , qemu-devel@nongnu.org, Stefan Hajnoczi , Peter Maydell On Tue, 18 Oct 2016 16:31:24 +0100 "Daniel P. Berrange" 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 > > > > --- > > > > 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