From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Eisele Subject: Re: [PATCH] depend.c: build up a dependency tree from c entities downto tokens: entries in the tree are: macro-depend: tree of #if nesting macro-expansions: possible macro expansion source of a token tok->macro-expansions->macro tok->macro-depend->ma Date: Thu, 26 Apr 2012 14:06:44 +0200 Message-ID: <4F993A54.3020706@gmail.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from mail-lb0-f174.google.com ([209.85.217.174]:34950 "EHLO mail-lb0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752598Ab2DZMD1 (ORCPT ); Thu, 26 Apr 2012 08:03:27 -0400 Received: by lbbgf7 with SMTP id gf7so906086lbb.19 for ; Thu, 26 Apr 2012 05:03:25 -0700 (PDT) In-Reply-To: Sender: linux-sparse-owner@vger.kernel.org List-Id: linux-sparse@vger.kernel.org To: Christopher Li Cc: linux-sparse@vger.kernel.org, sam@ravnborg.org, davem@davemloft.net, KE On 04/26/2012 11:13 AM, Christopher Li wrote: > . > On Wed, Apr 25, 2012 at 1:16 PM, Konrad Eisele wrote: >> Hello Christopher Li, >> I'd like to send a patch for review to be included >> into sparse. The patch builds up a dependency >> tree from c parse entities downto single token, >> making it possible to write tools that analyse what >> token was generated by what macro and which >> macros it is dependent of. Thanks for the reply, I'm open for rewrite. > > Sorry I just get back from a trip, did not have a chance to > write a reply during the travel. > > I briefly look through the patch, haven't got enough time > to look at it in the detail level. Here is a few things that > jump out, I am not going to apply this patch as it is. > >>> +void token_allocator_nofree(void) >>> +{ >>> + token_allocator.nofree = 1; >>> +} > > What is up with this nofree thing? > There should be much better way to keep the token > unfree. The thing is that you want to maintain the dependency from c-parsing units downto tokens. So the struct token should not be free'd after the parsing stage. Also calls to free_token should actually preserve the struct token so that it still can be referenced. > >>> diff --git a/allocate.h b/allocate.h >>> index 9f1dc8c..de85320 100644 >>> --- a/allocate.h >>> +++ b/allocate.h >>> @@ -12,6 +12,7 @@ struct allocator_struct { >>> struct allocation_blob *blobs; >>> unsigned int alignment; >>> unsigned int chunking; >>> + unsigned int nofree; > > Nack here as well. See above. > >>> diff --git a/attr.c b/attr.c >>> new file mode 100644 >>> index 0000000..54c4c21 >>> --- /dev/null >>> +++ b/attr.c >>> +#define HASH_LEN (1024*4) >>> +struct hash { >>> + struct hash_v *f; >>> +} h[HASH_LEN]; >>> + >>> --- /dev/null >>> +++ b/depend.c >>> @@ -0,0 +1,329 @@ >>> +/* >>> + * Build macros dependency tree >>> + * Copyright (C) 2012 Konrad Eisele >>> + * BSD-License >>> + * Redistribution and use in source and binary forms are permitted >>> + * provided that the above copyright notice and this paragraph are >>> + * duplicated in all such forms and that any documentation, >>> + * advertising materials, and other materials related to such >>> + * distribution and use acknowledge that the software was developed >>> + * by the. The name of the >>> + * University may not be used to endorse or promote products derived >>> + * from this software without specific prior written permission. >>> + * THIS SOFTWARE IS PROVIDED ``AS IS'' AND WITHOUT ANY EXPRESS OR >>> + * IMPLIED WARRANTIES, INCLUDING, WITHOUT LIMITATION, THE IMPLIED >>> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE >>> + */ > > Why BSD license? Sparse current is open software license and moving to > MIT license. I can set in any License you want. What I dont want is having to go through the FSF copyleft bureaucracy. BSD is the easiest I guess, but I'll set in any text. > >>> - struct expression *e = alloc_expression(token->pos, EXPR_STATEMENT); >>> + struct expression *e = alloc_expression_tok(token, EXPR_STATEMENT); > > Please don't do change like this. It does not have any add on value. Again, to build up the dependency tree going from c-parse entities downto tokens I need the struct token, not "pos". It doesnt change any logic, and the changes I did to the original soucecode are minimal that way. The other alternative is of course to switch from "pos" to struct token in all structures. (And retaint the tokens by default). > > > There are a lot more I want to comment on but I need some time to think about > what is the best way to do it. I am not oppose to adding feature to track macro > dependency, but it need to be less intrusive to the code. I want to avoid impact > the normal code path that does not care about macro dependency. e.g. the > sparse checker. > > I also suspect the hashing is unnecessary. Give me some time to come up > with some suggestion. Without the hashing quite a lot of rewrite of the internal structures are needed, with the hashing the original structures can be maintained, which are simple and easy. The dependency tree caries a lot of extra data with it. "Normal" cases dont need that. For instance you want for each token pointers to: "Macro dependency-tree" + "Macro expansion" + "endpos" + "visited" etc. And even on my slow laptop no difference in speed is noticable with hashing. An middle way would be to add to each structure a "void *custom_data" that could be used/extend by the application. Then the data structures get a bit bigger however you dont need to traverse the hash tree. -- Konrad > > Chris >