IMA: backward-compatible behavior from lhd_set_decl_assembler_name

IMA: backward-compatible behavior from lhd_set_decl_assembler_name

[Zack Weinberg]

One of the biggest stumbling blocks to re-enabling IMA has been the C
front end's special set_decl_assembler_name hook.  It exists because
the generic hook isn't aware of the special significance of having
DECL_CONTEXT point to a TRANSLATION_UNIT_DECL.  The generic hook's
behavior is formally correct when DECL_CONTEXT points to a T_U_D, but
is not backward compatible for file-scope declarations with internal
linkage.  Theoretically, nothing should care what the object-file
symbol name of such a declaration is; in practice, people have scripts
that munge the .s file, or the nm output, or whatever, that break.
Therefore the C front end overrides that with backward-compatible
behavior when there's just one input file in play.

The problem with all this is that the C front end's override hook only
works properly if *every* declaration with internal linkage has its
DECL_CONTEXT set at the point when the hook is called.  I've spent the
past two weeks trying to make that be true, and I got maybe 96% there,
but it just doesn't look feasible to do for the remaining 4% (which
are mainly synthetic declarations created by language-independent code
that doesn't have the ability to hook into the C front end's scoping
structures - nor should it have to).  So instead, this patch
rearranges the generic hook to give backward-compatible behavior
without help from the C front end.

The new behavior is: Declarations with internal linkage at file scope in
the first input file get DECL_ASSEMBLER_NAME set equal to DECL_NAME.
This is what the aforementioned scripts expect.  Declarations with
internal linkage at file scope in the second and successive input files
get DECL_NAME + "." + a number; the number chosen is the DECL_UID of the
containing TRANSLATION_UNIT_DECL.  This number will always be in the
range 1 ... num_in_fnames-1; make_node and so on are adjusted such that
DECL_UIDs 0 through num_in_fnames are reserved for TRANSLATION_UNIT_DECLs.

Declarations with internal linkage at block scope also get a numeric
suffix, which is what we've always done with them, but I changed it from
a sequence number to the declaration's own DECL_UID, which allowed me to
remove PCH's grubby little fingers from langhooks.c.  (Technically we
could use the declaration's own DECL_UID for the file-scope declarations
in the second and successive input files as well, but using the T_U_D's
number is a useful sanity check that the language hasn't messed up and
created two declarations with the same name -- it's already constrained
not to do so in the first input file, and we want consistency.)

Internal linkage declarations with null DECL_CONTEXT also get no numeric
suffix.  For C, this should only happen for synthetic declarations
created by language-independent code (e.g. cgraph_build_static_cdtor).
I believe this to be harmless wrt IMA - these things tend to be created
just once in the entire object file, anyway.

The only wart is that reserving the first few DECL_UIDs for
TRANSLATION_UNIT_DECLs places a new constraint on front ends: they must
not create any DECL nodes prior to calling build_common_tree_nodes.  The
only front end that did that was C++ and it didn't have to; I just moved
the call to build_common_tree_nodes a few lines up in cxx_init.  The
*proper* fix here would be to call build_common_tree_nodes from
language-independent code, just before lang_hooks.init, but that would
require me to untangle flag_signed_char which is too tangential.

Bootstrapped i686-linux (all languages but Ada); committed.

zw

        * langhooks.c: Don't include gt-langhooks.h.
        (var_labelno): Delete.
        (lhd_set_decl_assembler_name): Do not append a distinguishing
        number to file-scope internal-linkage declarations for the first
        input file, even if they have DECL_CONTEXT set.  Use DECL_UID of
        the declaration itself (if at block scope), or its containing 
        TRANSLATION_UNIT_DECL (if at file scope) for the distinguishing
        number.

        * opts.c (cur_in_fname): New global.
        * opts.h: Declare it.
        * tree.c: Include opts.h.
        (make_node_stat): If creating a TRANSLATION_UNIT_DECL, give it
        DECL_UID equal to cur_in_fname.
        (copy_node_stat): Do not change DECL_UID when copying a
        TRANSLATION_UNIT_DECL.
        (build_common_tree_nodes): Adjust next_decl_uid to reserve the
        range 0 .. num_in_fnames-1 for TRANSLATION_UNIT_DECLs.

        * c-decl.c (c_static_assembler_name): Delete.
        * c-tree.h (c_static_assembler_name): Delete prototype.
        * c-lang.c, objc/objc-lang.c: Don't override
        LANG_HOOKS_SET_DECL_ASSEMBLER_NAME.

        * Makefile.in (tree.o): Update dependencies.
        (GTFILES): Remove langhooks.c.

cp:
        * decl.c (cxx_init_decl_processing): Call
        build_common_tree_nodes before creating the global NAMESPACE_DECL.

===================================================================
Index: Makefile.in
--- Makefile.in	4 Jul 2004 17:18:59 -0000	1.1317
+++ Makefile.in	5 Jul 2004 17:04:17 -0000
@@ -1569,7 +1569,7 @@ langhooks.o : langhooks.c $(CONFIG_H) $(
    $(LANGHOOKS_DEF_H) $(FLAGS_H) $(GGC_H) gt-langhooks.h diagnostic.h
 tree.o : tree.c $(CONFIG_H) $(SYSTEM_H) coretypes.h $(TM_H) $(TREE_H) $(FLAGS_H) function.h \
    toplev.h $(GGC_H) $(HASHTAB_H) $(TARGET_H) output.h $(TM_P_H) langhooks.h \
-   real.h gt-tree.h tree-iterator.h $(BASIC_BLOCK_H) $(TREE_FLOW_H)
+   real.h gt-tree.h tree-iterator.h $(BASIC_BLOCK_H) $(TREE_FLOW_H) opts.h
 tree-dump.o: tree-dump.c $(CONFIG_H) $(SYSTEM_H) coretypes.h $(TM_H) $(TREE_H) \
    $(C_TREE_H) $(FLAGS_H) langhooks.h toplev.h output.h c-pragma.h $(RTL_H) \
    $(GGC_H) $(EXPR_H) $(SPLAY_TREE_H) $(TREE_DUMP_H) tree-iterator.h
@@ -2337,7 +2337,7 @@ GTFILES = $(srcdir)/input.h $(srcdir)/co
   $(srcdir)/fold-const.c $(srcdir)/function.c \
   $(srcdir)/gcse.c $(srcdir)/integrate.c $(srcdir)/lists.c $(srcdir)/optabs.c \
   $(srcdir)/profile.c $(srcdir)/ra-build.c $(srcdir)/regclass.c \
-  $(srcdir)/reg-stack.c $(srcdir)/cfglayout.c $(srcdir)/langhooks.c \
+  $(srcdir)/reg-stack.c $(srcdir)/cfglayout.c \
   $(srcdir)/sdbout.c $(srcdir)/stmt.c $(srcdir)/stor-layout.c \
   $(srcdir)/stringpool.c $(srcdir)/tree.c $(srcdir)/varasm.c \
   $(srcdir)/tree-mudflap.c $(srcdir)/tree-flow.h \
===================================================================
Index: c-decl.c
--- c-decl.c	3 Jul 2004 02:35:05 -0000	1.525
+++ c-decl.c	5 Jul 2004 17:04:17 -0000
@@ -6576,22 +6576,6 @@ make_pointer_declarator (tree type_quals
   return build1 (INDIRECT_REF, quals, itarget);
 }
 
-/* A wrapper around lhd_set_decl_assembler_name that gives static
-   variables their C names if they are at file scope and only one
-   translation unit is being compiled, for backwards compatibility
-   with certain bizarre assembler hacks (like crtstuff.c).  */
-
-void
-c_static_assembler_name (tree decl)
-{
-  if (num_in_fnames == 1
-      && !TREE_PUBLIC (decl) && DECL_CONTEXT (decl)
-      && TREE_CODE (DECL_CONTEXT (decl)) == TRANSLATION_UNIT_DECL)
-    SET_DECL_ASSEMBLER_NAME (decl, DECL_NAME (decl));
-  else
-    lhd_set_decl_assembler_name (decl);
-}
-
 /* Perform final processing on file-scope data.  */
 static void
 c_write_global_declarations_1 (tree globals)
===================================================================
Index: c-lang.c
--- c-lang.c	1 Jul 2004 08:52:25 -0000	1.128
+++ c-lang.c	5 Jul 2004 17:04:17 -0000
@@ -76,8 +76,6 @@ enum c_language_kind c_language = clk_c;
 #define LANG_HOOKS_UNSAFE_FOR_REEVAL c_common_unsafe_for_reeval
 #undef LANG_HOOKS_STATICP
 #define LANG_HOOKS_STATICP c_staticp
-#undef LANG_HOOKS_SET_DECL_ASSEMBLER_NAME
-#define LANG_HOOKS_SET_DECL_ASSEMBLER_NAME c_static_assembler_name
 #undef LANG_HOOKS_NO_BODY_BLOCKS
 #define LANG_HOOKS_NO_BODY_BLOCKS true
 #undef LANG_HOOKS_WARN_UNUSED_GLOBAL_DECL
===================================================================
Index: c-tree.h
--- c-tree.h	5 Jul 2004 09:35:23 -0000	1.160
+++ c-tree.h	5 Jul 2004 17:04:17 -0000
@@ -189,7 +189,6 @@ extern tree start_struct (enum tree_code
 extern void store_parm_decls (void);
 extern tree xref_tag (enum tree_code, tree);
 extern int c_expand_decl (tree);
-extern void c_static_assembler_name (tree);
 extern tree make_pointer_declarator (tree, tree);
 
 /* in c-objc-common.c */
===================================================================
Index: langhooks.c
--- langhooks.c	24 Jun 2004 22:45:57 -0000	1.65
+++ langhooks.c	5 Jul 2004 17:04:17 -0000
@@ -157,11 +157,6 @@ lhd_warn_unused_global_decl (tree decl)
   return true;
 }
 
-/* Number for making the label on the next
-   static variable internal to a function.  */
-
-static GTY(()) int var_labelno;
-
 /* Set the DECL_ASSEMBLER_NAME for DECL.  */
 void
 lhd_set_decl_assembler_name (tree decl)
@@ -184,18 +179,33 @@ lhd_set_decl_assembler_name (tree decl)
 
          Can't use just the variable's own name for a variable whose
 	 scope is less than the whole compilation.  Concatenate a
-	 distinguishing number.  */
-      if (!TREE_PUBLIC (decl) && DECL_CONTEXT (decl))
+	 distinguishing number.  If the decl is at block scope, the
+	 number assigned is the DECL_UID; if the decl is at file
+	 scope, the number is the DECL_UID of the surrounding
+	 TRANSLATION_UNIT_DECL, except for the T_U_D with UID 0.
+	 Those (the file-scope internal-linkage declarations from the
+	 first input file) get no suffix, which is consistent with
+	 what has historically been done for file-scope declarations
+	 with internal linkage.  */
+      if (TREE_PUBLIC (decl)
+	  || DECL_CONTEXT (decl) == NULL_TREE
+	  || (TREE_CODE (DECL_CONTEXT (decl)) == TRANSLATION_UNIT_DECL
+	      && DECL_UID (DECL_CONTEXT (decl)) == 0))
+	SET_DECL_ASSEMBLER_NAME (decl, DECL_NAME (decl));
+      else
 	{
 	  const char *name = IDENTIFIER_POINTER (DECL_NAME (decl));
 	  char *label;
-	  
-	  ASM_FORMAT_PRIVATE_NAME (label, name, var_labelno);
-	  var_labelno++;
+	  unsigned int uid;
+
+	  if (TREE_CODE (DECL_CONTEXT (decl)) == TRANSLATION_UNIT_DECL)
+	    uid = DECL_UID (DECL_CONTEXT (decl));
+	  else
+	    uid = DECL_UID (decl);
+
+	  ASM_FORMAT_PRIVATE_NAME (label, name, uid);
 	  SET_DECL_ASSEMBLER_NAME (decl, get_identifier (label));
 	}
-      else
-	SET_DECL_ASSEMBLER_NAME (decl, DECL_NAME (decl));
     }
   else
     /* Nobody should ever be asking for the DECL_ASSEMBLER_NAME of
@@ -581,5 +591,3 @@ lhd_make_node (enum tree_code code)
 {
   return make_node (code);
 }
-
-#include "gt-langhooks.h"
===================================================================
Index: opts.c
--- opts.c	29 Jun 2004 01:53:02 -0000	1.71
+++ opts.c	5 Jul 2004 17:04:17 -0000
@@ -92,6 +92,7 @@ static bool flag_peel_loops_set, flag_br
 /* Input file names.  */
 const char **in_fnames;
 unsigned num_in_fnames;
+unsigned cur_in_fname;
 
 static size_t find_opt (const char *, int);
 static int common_handle_option (size_t scode, const char *arg, int value);
===================================================================
Index: opts.h
--- opts.h	14 Jun 2004 14:17:56 -0000	1.14
+++ opts.h	5 Jul 2004 17:04:17 -0000
@@ -57,4 +57,8 @@ extern const char **in_fnames;
 
 extern unsigned num_in_fnames;
 
+/* Current input filename index.  */
+
+extern unsigned cur_in_fname;
+
 #endif
===================================================================
Index: tree.c
--- tree.c	5 Jul 2004 09:35:26 -0000	1.387
+++ tree.c	5 Jul 2004 17:04:17 -0000
@@ -48,6 +48,7 @@ Software Foundation, 59 Temple Place - S
 #include "tree-iterator.h"
 #include "basic-block.h"
 #include "tree-flow.h"
+#include "opts.h"
 
 /* obstack.[ch] explicitly declined to prototype this.  */
 extern int _obstack_allocated_p (struct obstack *h, void *obj);
@@ -309,7 +310,10 @@ make_node_stat (enum tree_code code MEM_
       DECL_USER_ALIGN (t) = 0;
       DECL_IN_SYSTEM_HEADER (t) = in_system_header;
       DECL_SOURCE_LOCATION (t) = input_location;
-      DECL_UID (t) = next_decl_uid++;
+      if (code == TRANSLATION_UNIT_DECL)
+	DECL_UID (t) = cur_in_fname;
+      else
+	DECL_UID (t) = next_decl_uid++;
 
       /* We have not yet computed the alias set for this declaration.  */
       DECL_POINTER_ALIAS_SET (t) = -1;
@@ -382,7 +386,7 @@ copy_node_stat (tree node MEM_STAT_DECL)
   TREE_VISITED (t) = 0;
   t->common.ann = 0;
 
-  if (TREE_CODE_CLASS (code) == 'd')
+  if (TREE_CODE_CLASS (code) == 'd' && code != TRANSLATION_UNIT_DECL)
     DECL_UID (t) = next_decl_uid++;
   else if (TREE_CODE_CLASS (code) == 't')
     {
@@ -5310,6 +5314,14 @@ make_or_reuse_type (unsigned size, int u
 void
 build_common_tree_nodes (int signed_char)
 {
+  /* This function is called after command line parsing is complete,
+     but before any DECL nodes should have been created.  Therefore,
+     now is the appropriate time to adjust next_decl_uid so that the
+     range 0 .. num_in_fnames-1 is reserved for TRANSLATION_UNIT_DECLs.  */
+  if (next_decl_uid)
+    abort ();
+  next_decl_uid = num_in_fnames;
+
   error_mark_node = make_node (ERROR_MARK);
   TREE_TYPE (error_mark_node) = error_mark_node;
 
===================================================================
Index: cp/decl.c
--- cp/decl.c	2 Jul 2004 21:59:42 -0000	1.1233
+++ cp/decl.c	5 Jul 2004 17:04:18 -0000
@@ -2887,6 +2887,8 @@ cxx_init_decl_processing (void)
   tree void_ftype;
   tree void_ftype_ptr;
 
+  build_common_tree_nodes (flag_signed_char);
+
   /* Create all the identifiers we need.  */
   initialize_predefined_identifiers ();
 
@@ -2926,8 +2928,6 @@ cxx_init_decl_processing (void)
   /* Initially, C.  */
   current_lang_name = lang_name_c;
 
-  build_common_tree_nodes (flag_signed_char);
-
   error_mark_list = build_tree_list (error_mark_node, error_mark_node);
   TREE_TYPE (error_mark_list) = error_mark_node;
 
===================================================================
Index: objc/objc-lang.c
--- objc/objc-lang.c	1 Jul 2004 08:52:27 -0000	1.46
+++ objc/objc-lang.c	5 Jul 2004 17:04:18 -0000
@@ -73,8 +73,6 @@ enum c_language_kind c_language = clk_ob
 #define LANG_HOOKS_UNSAFE_FOR_REEVAL c_common_unsafe_for_reeval
 #undef LANG_HOOKS_STATICP
 #define LANG_HOOKS_STATICP c_staticp
-#undef LANG_HOOKS_SET_DECL_ASSEMBLER_NAME
-#define LANG_HOOKS_SET_DECL_ASSEMBLER_NAME c_static_assembler_name
 #undef LANG_HOOKS_NO_BODY_BLOCKS
 #define LANG_HOOKS_NO_BODY_BLOCKS true
 #undef LANG_HOOKS_DUP_LANG_SPECIFIC_DECL

Re: IMA: backward-compatible behavior from lhd_set_decl_assembler_name

[Geoffrey Keating]

Zack Weinberg <zack@codesourcery.com> writes:

> One of the biggest stumbling blocks to re-enabling IMA has been the C
> front end's special set_decl_assembler_name hook.  It exists because
> the generic hook isn't aware of the special significance of having
> DECL_CONTEXT point to a TRANSLATION_UNIT_DECL.

No, there was no accident here and no lack of knowledge.  The entire
purpose of a TRANSLATION_UNIT_DECL is to invoke this behaviour in
the generic code.

> The new behavior is: Declarations with internal linkage at file scope in
> the first input file get DECL_ASSEMBLER_NAME set equal to DECL_NAME.
> This is what the aforementioned scripts expect.  Declarations with
> internal linkage at file scope in the second and successive input files
> get DECL_NAME + "." + a number; the number chosen is the DECL_UID of the
> containing TRANSLATION_UNIT_DECL.  This number will always be in the
> range 1 ... num_in_fnames-1; make_node and so on are adjusted such that
> DECL_UIDs 0 through num_in_fnames are reserved for TRANSLATION_UNIT_DECLs.

I considered implementing something like this, but I rejected it,
because of:

---a.c---
static int x = 5;
int y(void) { return x++; }
---b.c---
extern int x;
int z(void) { return x++; }
---c.c---
int x = 2;
extern int y(void);
extern int z(void);
int main(void)
{
  if (y() + z() != 7)
    abort();
  exit(0);

$ gcc a.c b.c c.c -c -o a.o
$ gcc a.o -o a
$ ./a

$ gcc a.c b.c -c -o a2.o
$ gcc a2.o c.c -o a2
$ ./a2

I really think it would be better for you to send your designs to the list
for review before you start coding.  That way, things like this can be
caught early.

I presume you'll now back your patch out.

...
> Internal linkage declarations with null DECL_CONTEXT also get no numeric
> suffix.  For C, this should only happen for synthetic declarations
> created by language-independent code (e.g. cgraph_build_static_cdtor).
> I believe this to be harmless wrt IMA - these things tend to be created
> just once in the entire object file, anyway.

This is what has always been done, so it should work as well as it
always did.

> The only wart is that reserving the first few DECL_UIDs for
> TRANSLATION_UNIT_DECLs places a new constraint on front ends: they must
> not create any DECL nodes prior to calling build_common_tree_nodes.  The
> only front end that did that was C++ and it didn't have to; I just moved
> the call to build_common_tree_nodes a few lines up in cxx_init.  The
> *proper* fix here would be to call build_common_tree_nodes from
> language-independent code, just before lang_hooks.init, but that would
> require me to untangle flag_signed_char which is too tangential.

Why are you insisting that the DECL_UIDs for TRANSLATION_UNIT_DECLs
(past the first) must be small?  Is it just for convenience of someone
reading the assembly?

> Bootstrapped i686-linux (all languages but Ada); committed.

Re: IMA: backward-compatible behavior from lhd_set_decl_assembler_name

[Zack Weinberg]

Geoffrey Keating <geoffk@geoffk.org> writes:

> I considered implementing something like this, but I rejected it,
> because of:
>
> ---a.c---
> static int x = 5;
> int y(void) { return x++; }
> ---b.c---
> extern int x;
> int z(void) { return x++; }
> ---c.c---
> int x = 2;
> extern int y(void);
> extern int z(void);
> int main(void)
> {
>   if (y() + z() != 7)
>     abort();
>   exit(0);
>
> $ gcc a.c b.c c.c -c -o a.o
> $ gcc a.o -o a
> $ ./a
>
> $ gcc a.c b.c -c -o a2.o
> $ gcc a2.o c.c -o a2
> $ ./a2

I do not understand this objection.  Could you please explain (a) what
this is expected to do, and (b) why it won't work with my patch?

> I really think it would be better for you to send your designs to the list
> for review before you start coding.  That way, things like this can be
> caught early.
>
> I presume you'll now back your patch out.

If I am convinced that there is an actual problem, of course I will.

zw

Re: IMA: backward-compatible behavior from lhd_set_decl_assembler_name

[Geoff Keating]

On 05/07/2004, at 12:55 PM, Zack Weinberg wrote:

> Geoffrey Keating <geoffk@geoffk.org> writes:
>
>> I considered implementing something like this, but I rejected it,
>> because of:
>>
>> ---a.c---
>> static int x = 5;
>> int y(void) { return x++; }
>> ---b.c---
>> extern int x;
>> int z(void) { return x++; }
>> ---c.c---
>> int x = 2;
>> extern int y(void);
>> extern int z(void);
>> int main(void)
>> {
>>   if (y() + z() != 7)
>>     abort();
>>   exit(0);
>>
>> $ gcc a.c b.c c.c -c -o a.o
>> $ gcc a.o -o a
>> $ ./a
>>
>> $ gcc a.c b.c -c -o a2.o
>> $ gcc a2.o c.c -o a2
>> $ ./a2
>
> I do not understand this objection.  Could you please explain (a) what
> this is expected to do, and (b) why it won't work with my patch?

This is expected to compile and not abort(), since it is strictly 
conforming C (if you fix the closing brace).  According to your 
description of the patch:

> Declarations with internal linkage at file scope in the first input 
> file get DECL_ASSEMBLER_NAME set equal to DECL_NAME.

So, in a.c, 'x' will get DECL_ASSEMBLER_NAME of 'x', since a.c is the 
first input file.

Now, I presume that declarations with external linkage will continue to 
get their own name, too, so in b.c and c.c, 'x' will also get 
DECL_ASSEMBLER_NAME of 'x'.  (They'd better have the same name, since 
they are the same object.)

This is a problem, since the 'x' in a.c and the one in c.c are 
different objects.  I expect that this code won't compile under IMA; 
the assembler will choke.

Re: IMA: backward-compatible behavior from lhd_set_decl_assembler_name

[Zack Weinberg]

Geoff Keating <geoffk@geoffk.org> writes:
[...]
> This is expected to compile and not abort(), since it is strictly
> conforming C (if you fix the closing brace).  According to your
> description of the patch:
>
>> Declarations with internal linkage at file scope in the first input
>> file get DECL_ASSEMBLER_NAME set equal to DECL_NAME.
>
> So, in a.c, 'x' will get DECL_ASSEMBLER_NAME of 'x', since a.c is the
> first input file.
>
> Now, I presume that declarations with external linkage will continue
> to get their own name, too, so in b.c and c.c, 'x' will also get
> DECL_ASSEMBLER_NAME of 'x'.  (They'd better have the same name, since
> they are the same object.)
>
> This is a problem, since the 'x' in a.c and the one in c.c are
> different objects.  I expect that this code won't compile under IMA;
> the assembler will choke.

Thanks for the explanation.  I figured out what you meant about 15
minutes after sending the previous message, but at that point I had
already left the house and it was too late to say so.  (Had to spend
all day helping friends move.)

You're right, this won't work.  I've thought of three possible fixes.
All entail reverting most of the behavior change in
lhd_set_decl_assembler_name (but I'd prefer to keep the bit where we
use the DECL_UID for the disambiguating suffix.)

1) Put back c_static_assembler_name.  Hope that all the places where
   DECL_CONTEXT wasn't getting set right have been found.

2) Don't put back c_static_assembler_name, instead have pop_scope give
   !TREE_PUBLIC file-scope decls a NULL DECL_CONTEXT when there's only
   one translation unit in play, thus getting backward-compatible
   behavior from the default hook.  Again we have to hope that all the
   places where DECL_CONTEXT wasn't getting set right have been found.

3) Again don't put back c_static_assembler_name.  Have
   c_write_global_declarations scan all declarations in all file
   scopes and the externals scope, and clear DECL_CONTEXT for all
   !TREE_PUBLIC file-scope decls except those which actually need
   disambiguation (i.e. either two !TREE_PUBLIC decls with the same
   name appear in different translation units, or a !TREE_PUBLIC decl
   appears in one TU and a TREE_PUBLIC decl with the same name appears
   in the external scope).  This guarantees that DECL_CONTEXT is
   correct for everything, but does involve adding a big chunk of
   code, two more iterations over all file-scope declarations, and
   probably using tree_common flags on IDENTIFIER_NODE, which we're
   trying to get away from.

I think that all the places where DECL_CONTEXT wasn't getting set
right have in fact been found, so I currently prefer option 2.  What
do you think?

zw

Re: IMA: backward-compatible behavior from lhd_set_decl_assembler_name

[Geoff Keating]

On 05/07/2004, at 11:41 PM, Zack Weinberg wrote:

> Geoff Keating <geoffk@geoffk.org> writes:
> [...]
>> This is expected to compile and not abort(), since it is strictly
>> conforming C (if you fix the closing brace).  According to your
>> description of the patch:
>>
>>> Declarations with internal linkage at file scope in the first input
>>> file get DECL_ASSEMBLER_NAME set equal to DECL_NAME.
>>
>> So, in a.c, 'x' will get DECL_ASSEMBLER_NAME of 'x', since a.c is the
>> first input file.
>>
>> Now, I presume that declarations with external linkage will continue
>> to get their own name, too, so in b.c and c.c, 'x' will also get
>> DECL_ASSEMBLER_NAME of 'x'.  (They'd better have the same name, since
>> they are the same object.)
>>
>> This is a problem, since the 'x' in a.c and the one in c.c are
>> different objects.  I expect that this code won't compile under IMA;
>> the assembler will choke.
>
> Thanks for the explanation.  I figured out what you meant about 15
> minutes after sending the previous message, but at that point I had
> already left the house and it was too late to say so.  (Had to spend
> all day helping friends move.)
>
> You're right, this won't work.  I've thought of three possible fixes.
> All entail reverting most of the behavior change in
> lhd_set_decl_assembler_name (but I'd prefer to keep the bit where we
> use the DECL_UID for the disambiguating suffix.)
>
> 1) Put back c_static_assembler_name.  Hope that all the places where
>    DECL_CONTEXT wasn't getting set right have been found.
>
> 2) Don't put back c_static_assembler_name, instead have pop_scope give
>    !TREE_PUBLIC file-scope decls a NULL DECL_CONTEXT when there's only
>    one translation unit in play, thus getting backward-compatible
>    behavior from the default hook.  Again we have to hope that all the
>    places where DECL_CONTEXT wasn't getting set right have been found.
>
> 3) Again don't put back c_static_assembler_name.  Have
>    c_write_global_declarations scan all declarations in all file
>    scopes and the externals scope, and clear DECL_CONTEXT for all
>    !TREE_PUBLIC file-scope decls except those which actually need
>    disambiguation (i.e. either two !TREE_PUBLIC decls with the same
>    name appear in different translation units, or a !TREE_PUBLIC decl
>    appears in one TU and a TREE_PUBLIC decl with the same name appears
>    in the external scope).  This guarantees that DECL_CONTEXT is
>    correct for everything, but does involve adding a big chunk of
>    code, two more iterations over all file-scope declarations, and
>    probably using tree_common flags on IDENTIFIER_NODE, which we're
>    trying to get away from.
>
> I think that all the places where DECL_CONTEXT wasn't getting set
> right have in fact been found, so I currently prefer option 2.  What
> do you think?

I considered pretty much those exact options when I was writing the 
patch that created c_static_assembler_name.  I rejected option (3) as 
an unacceptable performance penalty in the single-file case.  I 
rejected option (2) because it adds extra paths in the C frontend which 
would be very lightly tested---to fully test the compiler, you would 
have to create an extra dejagnu C torture option that adds a trivial .c 
file to the compile command.

So I chose option (1), since it had no visible disadvantages.  I still 
think it doesn't, see below.


So, there's one thing I'm trying to understand.  You write:

> The problem with all this is that the C front end's override hook only
> works properly if every declaration with internal linkage has its
> DECL_CONTEXT set at the point when the hook is called.

What do you mean by 'properly'?  In particular, what

> synthetic declarations created by language-independent code

do you have that are bound to a particular translation unit?  
Language-independent code, by definition, does not know about 
translation units; not all languages have them like C, not even all 
languages that GCC compiles, consider Fortran.  So far as I know, all 
language-independent code:

1. Creates exactly one of whatever it creates, and therefore can safely 
use NULL DECL_CONTEXT; or
2. Creates many instances and has some way of naming them uniquely 
(like a counter) and therefore can also safely use NULL DECL_CONTEXT.

So I think that your statement about the C front end's override hook is 
not correct.

Related to this, your option (3) assumes that all declarations are 
available to c_write_global_declarations.  I do not know that is a safe 
assumption; the code that adds DECL_CONTEXT is the same code that adds 
declarations into the lists that c_static_assembler_name traverses, and 
I don't think it gets called for 'synthetic declarations'.  It was not 
a design requirement for c_write_global_declarations as I originally 
wrote it; indeed, I think I assumed the reverse, but I don't think it 
made any difference.


As a final question, is there something that you're not telling us?  
Based on the changes you've been making to the C frontend recently, it 
looks like you're working on some significant enhancement or new 
feature, because otherwise it makes no sense to make them; you seem to 
be spending a lot of time breaking things and fixing them again with 
little visible benefit.  If you don't want to discuss it on the list, 
it might help if you sent me private e-mail or even telephoned me.

Re: IMA: backward-compatible behavior from lhd_set_decl_assembler_name

[Mark Mitchell]

Zack Weinberg wrote:

>Geoff Keating <geoffk@geoffk.org> writes:
>[...]
>  
>
>>This is expected to compile and not abort(), since it is strictly
>>conforming C (if you fix the closing brace).  According to your
>>description of the patch:
>>
>>    
>>
>>>Declarations with internal linkage at file scope in the first input
>>>file get DECL_ASSEMBLER_NAME set equal to DECL_NAME.
>>>      
>>>
>>So, in a.c, 'x' will get DECL_ASSEMBLER_NAME of 'x', since a.c is the
>>first input file.
>>
>>Now, I presume that declarations with external linkage will continue
>>to get their own name, too, so in b.c and c.c, 'x' will also get
>>DECL_ASSEMBLER_NAME of 'x'.  (They'd better have the same name, since
>>they are the same object.)
>>
>>This is a problem, since the 'x' in a.c and the one in c.c are
>>different objects.  I expect that this code won't compile under IMA;
>>the assembler will choke.
>>    
>>
>
>Thanks for the explanation.  I figured out what you meant about 15
>minutes after sending the previous message, but at that point I had
>already left the house and it was too late to say so.  (Had to spend
>all day helping friends move.)
>
>You're right, this won't work.  I've thought of three possible fixes.
>All entail reverting most of the behavior change in
>lhd_set_decl_assembler_name (but I'd prefer to keep the bit where we
>use the DECL_UID for the disambiguating suffix.)
>
>1) Put back c_static_assembler_name.  Hope that all the places where
>   DECL_CONTEXT wasn't getting set right have been found.
>
>2) Don't put back c_static_assembler_name, instead have pop_scope give
>   !TREE_PUBLIC file-scope decls a NULL DECL_CONTEXT when there's only
>   one translation unit in play, thus getting backward-compatible
>   behavior from the default hook.  Again we have to hope that all the
>   places where DECL_CONTEXT wasn't getting set right have been found.
>
>3) Again don't put back c_static_assembler_name.  Have
>   c_write_global_declarations scan all declarations in all file
>   scopes and the externals scope, and clear DECL_CONTEXT for all
>   !TREE_PUBLIC file-scope decls except those which actually need
>   disambiguation (i.e. either two !TREE_PUBLIC decls with the same
>   name appear in different translation units, or a !TREE_PUBLIC decl
>   appears in one TU and a TREE_PUBLIC decl with the same name appears
>   in the external scope).  This guarantees that DECL_CONTEXT is
>   correct for everything, but does involve adding a big chunk of
>   code, two more iterations over all file-scope declarations, and
>   probably using tree_common flags on IDENTIFIER_NODE, which we're
>   trying to get away from.
>
>I think that all the places where DECL_CONTEXT wasn't getting set
>right have in fact been found, so I currently prefer option 2.  What
>do you think?
>
FWIW, I do think #2 sounds nice.  I think #3 sounds hard; probably #1 
would be easier than that.  If we can do without a hook, that's 
definitely nice.

-- 
Mark Mitchell
CodeSourcery, LLC
(916) 791-8304
mark@codesourcery.com

Re: IMA: backward-compatible behavior from lhd_set_decl_assembler_name

[Zack Weinberg]

Geoff Keating <geoffk@geoffk.org> writes:

> I considered pretty much those exact options when I was writing the
> patch that created c_static_assembler_name.  I rejected option (3) as
> an unacceptable performance penalty in the single-file case. 

I agree there.

> I rejected option (2) because it adds extra paths in the C frontend
> which would be very lightly tested---to fully test the compiler, you
> would have to create an extra dejagnu C torture option that adds a
> trivial .c file to the compile command.

I don't see how (1) and (2) are different here -- it's the same
lightly-tested extra path, but in a different place.  To verify this I
implemented (2) -- patch is attached; most of the bulk is reverting
the ill-advised parts of the previous patch -- and indeed it was a
matter of adding one clause to the controlling expression of an
if-statement that already existed.

I do agree that we've got a lightly-tested code path here, but I mean
to fix that by adding an IMA test suite at the same time that I
actually turn IMA back on (hopefully that will be tomorrow).

> So far as I know, all language-independent code:
>
> 1. Creates exactly one of whatever it creates, and therefore can
> safely use NULL DECL_CONTEXT; or
> 2. Creates many instances and has some way of naming them uniquely
> (like a counter) and therefore can also safely use NULL DECL_CONTEXT.

Ah, but no.  Mudflap, for instance, *thinks* it is creating one of its
things per object file, but because mudflap_finish_file is being
called from c_objc_common_finish_file, it is actually creating one of
its things per input translation unit, and since they all get the same
symbol name, the assembler barfs.

The cure for this, however, is to move the call to mudflap_finish_file
to a point where it will only be called once per object file - that's
the next patch in my queue, once we get this current issue resolved.
In principle you are correct that language-independent code can safely
use NULL DECL_CONTEXT - which is why I said in my original message
that I thought (2) was now safe.

The bulk of the work I have been doing to date is to stop the C front
end - language *dependent* code - from instantiating
DECL_ASSEMBLER_NAME too early, when DECL_CONTEXT for file-scope
statics is still null merely because the TRANSLATION_UNIT_DECL hasn't
been created yet.

> As a final question, is there something that you're not telling us?
> Based on the changes you've been making to the C frontend recently, it
> looks like you're working on some significant enhancement or new
> feature, because otherwise it makes no sense to make them; you seem to
> be spending a lot of time breaking things and fixing them again with
> little visible benefit.

Nope.  I am doing no more and no less than is necessary to fix IMA,
and you yourself warned me about most of the problems I have been
fixing, the ones where DECL_ASSEMBLER_NAME would be set too early.
Some of the patches should have compile-time performance benefits
independent of IMA, but that's not a current goal of mine (my
management has instructed me to finish fixing IMA first).

Here's my current candidate patch.  It gets no regressions on
i686-linux and I've checked a few object files to make sure the symbol
names are correct.

zw

        * c-decl.c (pop_scope): Do not set DECL_CONTEXT on file-scope
        decls when there is only one input translation unit.
        * langhooks.c (lhd_set_decl_assembler_name): Partially revert
        change of 2004-07-05; do not treat declarations with
        DECL_CONTEXT a TRANSLATION_UNIT_DECL specially.
        * opts.c (cur_in_fname): Delete.
        * opts.h: Likewise.
        * tree.c: Revert changes of 2004-07-05; no special treatment
        for TRANSLATION_UNIT_DECLs.
        * Makefile.in (tree.o): Update dependencies.

===================================================================
Index: Makefile.in
--- Makefile.in	6 Jul 2004 16:28:29 -0000	1.1320
+++ Makefile.in	7 Jul 2004 19:39:38 -0000
@@ -1569,7 +1569,7 @@ langhooks.o : langhooks.c $(CONFIG_H) $(
    $(LANGHOOKS_DEF_H) $(FLAGS_H) $(GGC_H) diagnostic.h
 tree.o : tree.c $(CONFIG_H) $(SYSTEM_H) coretypes.h $(TM_H) $(TREE_H) $(FLAGS_H) function.h \
    toplev.h $(GGC_H) $(HASHTAB_H) $(TARGET_H) output.h $(TM_P_H) langhooks.h \
-   real.h gt-tree.h tree-iterator.h $(BASIC_BLOCK_H) $(TREE_FLOW_H) opts.h
+   real.h gt-tree.h tree-iterator.h $(BASIC_BLOCK_H) $(TREE_FLOW_H)
 tree-dump.o: tree-dump.c $(CONFIG_H) $(SYSTEM_H) coretypes.h $(TM_H) $(TREE_H) \
    $(C_TREE_H) $(FLAGS_H) langhooks.h toplev.h output.h c-pragma.h $(RTL_H) \
    $(GGC_H) $(EXPR_H) $(SPLAY_TREE_H) $(TREE_DUMP_H) tree-iterator.h
===================================================================
Index: c-decl.c
--- c-decl.c	6 Jul 2004 02:20:05 -0000	1.528
+++ c-decl.c	7 Jul 2004 19:39:38 -0000
@@ -760,10 +760,12 @@ pop_scope (void)
 	      TREE_CHAIN (p) = BLOCK_VARS (block);
 	      BLOCK_VARS (block) = p;
 	    }
-	  /* If this is the file scope, must set DECL_CONTEXT on
-	     these.  Do so even for externals, so that
-	     same_translation_unit_p works.  */
-	  if (scope == file_scope)
+	  /* If this is the file scope, and we are processing more
+	     than one translation unit in this compilation, set
+	     DECL_CONTEXT of each decl to the TRANSLATION_UNIT_DECL.
+	     This makes same_translation_unit_p work, and causes
+	     static declarations to be given disambiguating suffixes.  */
+	  if (scope == file_scope && num_in_fnames > 1)
 	    DECL_CONTEXT (p) = context;
 
 	  /* Fall through.  */
===================================================================
Index: langhooks.c
--- langhooks.c	5 Jul 2004 17:28:33 -0000	1.66
+++ langhooks.c	7 Jul 2004 19:39:38 -0000
@@ -179,31 +179,15 @@ lhd_set_decl_assembler_name (tree decl)
 
          Can't use just the variable's own name for a variable whose
 	 scope is less than the whole compilation.  Concatenate a
-	 distinguishing number.  If the decl is at block scope, the
-	 number assigned is the DECL_UID; if the decl is at file
-	 scope, the number is the DECL_UID of the surrounding
-	 TRANSLATION_UNIT_DECL, except for the T_U_D with UID 0.
-	 Those (the file-scope internal-linkage declarations from the
-	 first input file) get no suffix, which is consistent with
-	 what has historically been done for file-scope declarations
-	 with internal linkage.  */
-      if (TREE_PUBLIC (decl)
-	  || DECL_CONTEXT (decl) == NULL_TREE
-	  || (TREE_CODE (DECL_CONTEXT (decl)) == TRANSLATION_UNIT_DECL
-	      && DECL_UID (DECL_CONTEXT (decl)) == 0))
+	 distinguishing number - we use the DECL_UID.  */
+      if (TREE_PUBLIC (decl) || DECL_CONTEXT (decl) == NULL_TREE)
 	SET_DECL_ASSEMBLER_NAME (decl, DECL_NAME (decl));
       else
 	{
 	  const char *name = IDENTIFIER_POINTER (DECL_NAME (decl));
 	  char *label;
-	  unsigned int uid;
 
-	  if (TREE_CODE (DECL_CONTEXT (decl)) == TRANSLATION_UNIT_DECL)
-	    uid = DECL_UID (DECL_CONTEXT (decl));
-	  else
-	    uid = DECL_UID (decl);
-
-	  ASM_FORMAT_PRIVATE_NAME (label, name, uid);
+	  ASM_FORMAT_PRIVATE_NAME (label, name, DECL_UID (decl));
 	  SET_DECL_ASSEMBLER_NAME (decl, get_identifier (label));
 	}
     }
===================================================================
Index: opts.c
--- opts.c	5 Jul 2004 17:28:33 -0000	1.72
+++ opts.c	7 Jul 2004 19:39:38 -0000
@@ -92,7 +92,6 @@ static bool flag_peel_loops_set, flag_br
 /* Input file names.  */
 const char **in_fnames;
 unsigned num_in_fnames;
-unsigned cur_in_fname;
 
 static size_t find_opt (const char *, int);
 static int common_handle_option (size_t scode, const char *arg, int value);
===================================================================
Index: opts.h
--- opts.h	5 Jul 2004 17:28:33 -0000	1.15
+++ opts.h	7 Jul 2004 19:39:38 -0000
@@ -57,8 +57,4 @@ extern const char **in_fnames;
 
 extern unsigned num_in_fnames;
 
-/* Current input filename index.  */
-
-extern unsigned cur_in_fname;
-
 #endif
===================================================================
Index: tree.c
--- tree.c	6 Jul 2004 02:20:09 -0000	1.389
+++ tree.c	7 Jul 2004 19:39:40 -0000
@@ -48,7 +48,6 @@ Software Foundation, 59 Temple Place - S
 #include "tree-iterator.h"
 #include "basic-block.h"
 #include "tree-flow.h"
-#include "opts.h"
 
 /* obstack.[ch] explicitly declined to prototype this.  */
 extern int _obstack_allocated_p (struct obstack *h, void *obj);
@@ -310,10 +309,7 @@ make_node_stat (enum tree_code code MEM_
       DECL_USER_ALIGN (t) = 0;
       DECL_IN_SYSTEM_HEADER (t) = in_system_header;
       DECL_SOURCE_LOCATION (t) = input_location;
-      if (code == TRANSLATION_UNIT_DECL)
-	DECL_UID (t) = cur_in_fname;
-      else
-	DECL_UID (t) = next_decl_uid++;
+      DECL_UID (t) = next_decl_uid++;
 
       /* We have not yet computed the alias set for this declaration.  */
       DECL_POINTER_ALIAS_SET (t) = -1;
@@ -386,7 +382,7 @@ copy_node_stat (tree node MEM_STAT_DECL)
   TREE_VISITED (t) = 0;
   t->common.ann = 0;
 
-  if (TREE_CODE_CLASS (code) == 'd' && code != TRANSLATION_UNIT_DECL)
+  if (TREE_CODE_CLASS (code) == 'd')
     DECL_UID (t) = next_decl_uid++;
   else if (TREE_CODE_CLASS (code) == 't')
     {
@@ -5341,14 +5337,6 @@ make_or_reuse_type (unsigned size, int u
 void
 build_common_tree_nodes (int signed_char)
 {
-  /* This function is called after command line parsing is complete,
-     but before any DECL nodes should have been created.  Therefore,
-     now is the appropriate time to adjust next_decl_uid so that the
-     range 0 .. num_in_fnames-1 is reserved for TRANSLATION_UNIT_DECLs.  */
-  if (next_decl_uid)
-    abort ();
-  next_decl_uid = num_in_fnames;
-
   error_mark_node = make_node (ERROR_MARK);
   TREE_TYPE (error_mark_node) = error_mark_node;
 

Re: IMA: backward-compatible behavior from lhd_set_decl_assembler_name

[Geoff Keating]

On 07/07/2004, at 12:44 PM, Zack Weinberg wrote:

> Geoff Keating <geoffk@geoffk.org> writes:
>
>> I considered pretty much those exact options when I was writing the
>> patch that created c_static_assembler_name.  I rejected option (3) as
>> an unacceptable performance penalty in the single-file case.
>
> I agree there.
>
>> I rejected option (2) because it adds extra paths in the C frontend
>> which would be very lightly tested---to fully test the compiler, you
>> would have to create an extra dejagnu C torture option that adds a
>> trivial .c file to the compile command.
>
> I don't see how (1) and (2) are different here -- it's the same
> lightly-tested extra path, but in a different place.

No!  (1) is five lines of code.  (2) is every place in the compiler 
that looks at DECL_CONTEXT.

>   To verify this I
> implemented (2) -- patch is attached; most of the bulk is reverting
> the ill-advised parts of the previous patch -- and indeed it was a
> matter of adding one clause to the controlling expression of an
> if-statement that already existed.
>
> I do agree that we've got a lightly-tested code path here, but I mean
> to fix that by adding an IMA test suite at the same time that I
> actually turn IMA back on (hopefully that will be tomorrow).



>> So far as I know, all language-independent code:
>>
>> 1. Creates exactly one of whatever it creates, and therefore can
>> safely use NULL DECL_CONTEXT; or
>> 2. Creates many instances and has some way of naming them uniquely
>> (like a counter) and therefore can also safely use NULL DECL_CONTEXT.
>
> Ah, but no.  Mudflap, for instance, *thinks* it is creating one of its
> things per object file, but because mudflap_finish_file is being
> called from c_objc_common_finish_file, it is actually creating one of
> its things per input translation unit, and since they all get the same
> symbol name, the assembler barfs.
>
> The cure for this, however, is to move the call to mudflap_finish_file
> to a point where it will only be called once per object file - that's
> the next patch in my queue, once we get this current issue resolved.
> In principle you are correct that language-independent code can safely
> use NULL DECL_CONTEXT - which is why I said in my original message
> that I thought (2) was now safe.
>
> The bulk of the work I have been doing to date is to stop the C front
> end - language *dependent* code - from instantiating
> DECL_ASSEMBLER_NAME too early, when DECL_CONTEXT for file-scope
> statics is still null merely because the TRANSLATION_UNIT_DECL hasn't
> been created yet.

Right.  That's why when I wrote IMA, I created the T_U_D before 
creating any decls, so that you don't have half-finished decls lying 
around.  You changed that as part of a fix to, I believe, PCH, which 
was broken by your previous fix to a bunch of problems, which itself 
was necessary because of a previous fix of yours.  I never did get an 
explanation as to what the original patch improved; I hope for your 
sake that it was worth it.

I believe I objected to at least half of those fixes.  Your designs 
were not sound, and you are not following good software engineering 
practises.  If you continue on this path, I expect that it will be 
months or years before we have C frontend that works in all the cases 
it used to.

>> As a final question, is there something that you're not telling us?
>> Based on the changes you've been making to the C frontend recently, it
>> looks like you're working on some significant enhancement or new
>> feature, because otherwise it makes no sense to make them; you seem to
>> be spending a lot of time breaking things and fixing them again with
>> little visible benefit.
>
> Nope.  I am doing no more and no less than is necessary to fix IMA,
> and you yourself warned me about most of the problems I have been
> fixing, the ones where DECL_ASSEMBLER_NAME would be set too early.
> Some of the patches should have compile-time performance benefits
> independent of IMA, but that's not a current goal of mine (my
> management has instructed me to finish fixing IMA first).

So, why on earth are you doing it?  Wouldn't it be much faster and much 
easier to revert back to the C frontend as it existed before you 
started changing it?

Re: IMA: backward-compatible behavior from lhd_set_decl_assembler_name

[Zack Weinberg]

Geoff Keating <geoffk@geoffk.org> writes:

>> I don't see how (1) and (2) are different here -- it's the same
>> lightly-tested extra path, but in a different place.
>
> No!  (1) is five lines of code.  (2) is every place in the compiler
> that looks at DECL_CONTEXT.

Did you actually look at the patch I sent you?

zw

Re: IMA: backward-compatible behavior from lhd_set_decl_assembler_name

[Geoff Keating]

On 07/07/2004, at 3:44 PM, Zack Weinberg wrote:

> Geoff Keating <geoffk@geoffk.org> writes:
>
>>> I don't see how (1) and (2) are different here -- it's the same
>>> lightly-tested extra path, but in a different place.
>>
>> No!  (1) is five lines of code.  (2) is every place in the compiler
>> that looks at DECL_CONTEXT.
>
> Did you actually look at the patch I sent you?

Yes, I did.  Are you claiming that your patch does not affect the 
contents of DECL_CONTEXT?

Re: IMA: backward-compatible behavior from lhd_set_decl_assembler_name

[Zack Weinberg]

Geoff Keating <geoffk@geoffk.org> writes:

> On 07/07/2004, at 3:44 PM, Zack Weinberg wrote:
>
>> Geoff Keating <geoffk@geoffk.org> writes:
>>
>>>> I don't see how (1) and (2) are different here -- it's the same
>>>> lightly-tested extra path, but in a different place.
>>>
>>> No!  (1) is five lines of code.  (2) is every place in the compiler
>>> that looks at DECL_CONTEXT.
>>
>> Did you actually look at the patch I sent you?
>
> Yes, I did.  Are you claiming that your patch does not affect the
> contents of DECL_CONTEXT?

No, I'm claiming that (2) does *not* affect every place in the
compiler that looks at DECL_CONTEXT, and I don't see how you could
possibly think that unless you didn't actually read my patch.

zw

Re: IMA: backward-compatible behavior from lhd_set_decl_assembler_name

[Geoff Keating]

On 07/07/2004, at 6:16 PM, Zack Weinberg wrote:

> Geoff Keating <geoffk@geoffk.org> writes:
>
>> On 07/07/2004, at 3:44 PM, Zack Weinberg wrote:
>>
>>> Geoff Keating <geoffk@geoffk.org> writes:
>>>
>>>>> I don't see how (1) and (2) are different here -- it's the same
>>>>> lightly-tested extra path, but in a different place.
>>>>
>>>> No!  (1) is five lines of code.  (2) is every place in the compiler
>>>> that looks at DECL_CONTEXT.
>>>
>>> Did you actually look at the patch I sent you?
>>
>> Yes, I did.  Are you claiming that your patch does not affect the
>> contents of DECL_CONTEXT?
>
> No, I'm claiming that (2) does *not* affect every place in the
> compiler that looks at DECL_CONTEXT, and I don't see how you could
> possibly think that unless you didn't actually read my patch.

Your patch included this chunk:

Index: c-decl.c
--- c-decl.c	6 Jul 2004 02:20:05 -0000	1.528
+++ c-decl.c	7 Jul 2004 19:39:38 -0000
@@ -760,10 +760,12 @@ pop_scope (void)
  	      TREE_CHAIN (p) = BLOCK_VARS (block);
  	      BLOCK_VARS (block) = p;
  	    }
-	  /* If this is the file scope, must set DECL_CONTEXT on
-	     these.  Do so even for externals, so that
-	     same_translation_unit_p works.  */
-	  if (scope == file_scope)
+	  /* If this is the file scope, and we are processing more
+	     than one translation unit in this compilation, set
+	     DECL_CONTEXT of each decl to the TRANSLATION_UNIT_DECL.
+	     This makes same_translation_unit_p work, and causes
+	     static declarations to be given disambiguating suffixes.  */
+	  if (scope == file_scope && num_in_fnames > 1)
  	    DECL_CONTEXT (p) = context;

Now, I may be misreading it, but doesn't this cause DECL_CONTEXT to be 
set, sometimes, to a T_U_D; but after your patch, only when IMA is in 
use?  And thus, any code that's looking at DECL_CONTEXT might work 
happily with it being NULL when not using IMA, and then break when it 
sees a T_U_D when not using IMA?

Re: IMA: backward-compatible behavior from lhd_set_decl_assembler_name

[Zack Weinberg]

Geoff Keating <geoffk@geoffk.org> writes:

> On 07/07/2004, at 6:16 PM, Zack Weinberg wrote:
>> No, I'm claiming that (2) does *not* affect every place in the
>> compiler that looks at DECL_CONTEXT, and I don't see how you could
>> possibly think that unless you didn't actually read my patch.
...

> Now, I may be misreading it, but doesn't this cause DECL_CONTEXT to be
> set, sometimes, to a T_U_D; but after your patch, only when IMA is in
> use?

Correct.

> And thus, any code that's looking at DECL_CONTEXT might work happily
> with it being NULL when not using IMA, and then break when it sees a
> T_U_D [when] using IMA?

Possible.  That is a different observation from the one I thought you
were originally making.  I would characterise it as "the IMA feature
introduces a code path which is rarely taken, and so may bitrot, so
let's artificially cause it to be used more often to reduce the risk
of bitrot."

Personally I would rather keep IMA working with a testsuite (which
Andrew Pinski has been kind enough to write, and I'll certainly add
your earlier test case).  If you're dead set on having DECL_CONTEXT of
file-scope DECLs be a TRANSLATION_UNIT_DECL whether or not IMA is in
use, I suppose I could put the special case for num_in_fnames==1 into
lhd_set_decl_assembler_name.

zw