This is the mail archive of the
gcc@gcc.gnu.org
mailing list for the GCC project.
Re: IMA corner case with forward declarations of statics
On Jun 23, 2004, at 8:07 PM, Zack Weinberg wrote:
Geoff Keating <geoffk@geoffk.org> writes:
On 23/06/2004, at 7:43 PM, Zack Weinberg wrote:
Consider the following test case:
A.c:
int foo = 12;
B.c:
extern int printf(const char *, ...);
extern int foo;
int main(void)
{
printf("%d\n", foo);
return 0;
}
static int foo = 14;
Taken together, this is a correct C program which should print 14.
Actually, this is a violation of 6.2.2 paragraph 7,
If, within a translation unit, the same identifier appears with both
internal and external linkage, the behavior is undefined.
Are you sure? 6.2.2p4 looks unambiguous that the first declaration
does have external linkage, but I remember finding a place that said
that a declaration with 'extern' and no initializer had indeterminate
linkage, which the subsequent declaration with 'static' retroactively
rendered into internal linkage.
This is correct, you're thinking of 6.9.2. Your example is valid.
Now, GCC 3.4's IMA doesn't do the right thing with this either; no
matter which order the files are given in, you get
Assembler messages:
Error: symbol `foo' is already defined
No one has noticed, which leads me to believe that this will not be a
problem in real life.
Actually, I did notice, shortly after your patch that broke IMA went
in. And I have a
fix for it in 3.3. (Obviously not useful in mainline, but could
probably be ported to 3.4
if anybody cares, which I doubt; as you say, nobody else noticed.)
I ran across this fixing IMA with -fno-common and -fno-unit-at-a-time,
both of which also didn't work in some cases. Whoever fixes this,
please test
those cases. 6.9.2 in the standard is a good source for things to try.
Diffs of most of my patch below, in case they're useful. The logic
changes substantially
to handle this case, but there's nothing impossible about it.
(DECL_DUPLICATE_DECL
is a new bit, and I have a vague memory it's set somewhere else as
well, but the
logic change shown here is the tricky bit.)
Index: c-decl.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/c-decl.c,v
retrieving revision 1.334.2.87.2.2
retrieving revision 1.334.2.87.2.3
diff -u -d -b -w -r1.334.2.87.2.2 -r1.334.2.87.2.3
--- c-decl.c 3 Apr 2004 02:54:01 -0000 1.334.2.87.2.2
+++ c-decl.c 23 Apr 2004 19:03:22 -0000 1.334.2.87.2.3
@@ -6670,7 +6673,37 @@
/* Propagate information between definitions and uses between multiple
translation units in TU_LIST based on linkage rules. */
+/* APPLE LOCAL rewritten to support -fno-common, no unit-at-a-time.
+ The general idea is to identify the unique definition, and mark
+ all the other declarations that refer to the same object as
duplicates.
+ varasm emits only non-duplicates. I think some of the problems are
actually
+ connected with unit-at-a-time rather than no-common, now that you
mention it.
+ The following used to fail and now work:
+
+file1:
+int i2 = 2; // optionally add 'extern'
+
+file 2:
+extern int i2; // optionally elide 'extern'
+
+cc file1.c file2.c
+FATAL: Symbol _i2 already defined.
+
+This is independent of -fno-common. The order of the files may be
interchanged.
+(With -fno-common, the variant where you omit 'extern' in file 2 is of
course an
+error, which was and still is detected correctly at compile time.)
+
+file 1:
+int i1;
+
+file 2:
+extern int i1;
+
+cc file1.c file2.c
+6: Ignoring attempt to redefine symbol.
+This used to fail with -fno-common, in either order.
+*/
void
merge_translation_unit_decls (void)
{
@@ -6693,23 +6726,43 @@
link_hash_table = htab_create (1021, link_hash_hash, link_hash_eq,
NULL);
- /* Enter any actual definitions into the hash table. */
+ /* Enter any actual definitions into the hash table.
+ For functions, ignore declarations. For variables, declarations
+ are kept in the table until a definition is found; duplicate
+ declarations are so marked, so that varasm will emit only one
+ of them. */
for (tu = tu_list; tu; tu = TREE_CHAIN (tu))
for (decl = BLOCK_VARS (DECL_INITIAL (tu)); decl; decl =
TREE_CHAIN (decl))
- if (TREE_PUBLIC (decl) && ! DECL_EXTERNAL (decl))
+ if (TREE_PUBLIC (decl) &&
+ (TREE_CODE (decl) == VAR_DECL || !DECL_EXTERNAL (decl)))
{
PTR *slot;
+ tree old_decl;
+
+ /* Insert in table. Declarations go in. */
slot = htab_find_slot (link_hash_table, decl, INSERT);
+ old_decl = *slot;
- /* If we've already got a definition, work out which one is
- the real one, put it into the hash table, and make the
- other one DECL_EXTERNAL. This is important to avoid
- putting out two definitions of the same symbol in the
- assembly output. */
- if (*slot != NULL)
+ if (old_decl == NULL)
{
- tree old_decl = (tree) *slot;
-
+ /* New definition or declaration. */
+ *slot = decl;
+ }
+ else if (old_decl == decl)
+ /* This currently gets called once per file (seems like
+ potentially a big time sink, but doesn't show up as
+ such), so this can happen. */
+ ;
+ /* If this is a definition and we've already got a definition,
+ work out which one is the real one, put it into the hash
+ table, and make the other one DECL_EXTERNAL. This is
+ important to avoid putting out two definitions of the
+ same symbol in the assembly output. */
+ else if (!DECL_EXTERNAL (old_decl) && !DECL_EXTERNAL (decl))
+ {
+ /* Should be from different files. */
+ if (DECL_CONTEXT (old_decl) == DECL_CONTEXT (decl))
+ abort ();
/* If this is weak or common or whatever, suppress it
in favor of the other definition. */
if (DECL_WEAK (decl))
@@ -6727,6 +6780,7 @@
DECL_COMMON (decl) = 0;
DECL_ONE_ONLY (decl) = 0;
DECL_WEAK (decl) = 0;
+ DECL_DUPLICATE_DECL (decl) = 1;
}
else if (DECL_EXTERNAL (old_decl))
{
@@ -6734,6 +6788,7 @@
DECL_COMMON (old_decl) = 0;
DECL_ONE_ONLY (old_decl) = 0;
DECL_WEAK (old_decl) = 0;
+ DECL_DUPLICATE_DECL (old_decl) = 1;
*slot = decl;
}
else
@@ -6742,9 +6797,21 @@
error ("%J'%D' previously defined here", old_decl,
old_decl);
}
}
- else
+ else if (DECL_EXTERNAL (old_decl))
+ {
+ /* Old entry is a declaration. Mark it duplicate and
+ replace with new decl (whether declaration or
+ definition). */
+ DECL_DUPLICATE_DECL (old_decl) = 1;
*slot = decl;
}
+ else
+ {
+ /* Old entry is definition, new one declaration.
+ Mark the declaration as a dup. */
+ DECL_DUPLICATE_DECL (decl) = 1;
+ }
+ }
/* Now insert the desired information from all the definitions
into any plain declarations. */
@@ -6772,6 +6839,7 @@
htab_delete (link_hash_table);
}
+/* APPLE LOCAL end rewrite */
/* Perform final processing on file-scope data. */
Index: varasm.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/varasm.c,v
retrieving revision 1.295.2.47.2.5
retrieving revision 1.295.2.47.2.6
diff -u -d -b -w -r1.295.2.47.2.5 -r1.295.2.47.2.6
--- varasm.c 16 Apr 2004 00:10:35 -0000 1.295.2.47.2.5
+++ varasm.c 23 Apr 2004 19:03:23 -0000 1.295.2.47.2.6
@@ -1440,6 +1440,10 @@
if (flag_syntax_only)
return;
+ /* APPLE LOCAL duplicate decls in multiple files. */
+ if (DECL_DUPLICATE_DECL (decl))
+ return;
+
app_disable ();
if (! dont_output_data