intermodule and comptypes - need some help

intermodule and comptypes - need some help

[Zack Weinberg]

A patch I'm working on has, as a side effect, that (in intermodule
mode) duplicate_decls may encounter two declarations from different
translation units at any time, not just during
merge_translation_unit_decls.  It therefore cannot simply be told when
to invoke comptypes with the special COMPARE_DIFFERENT_TU flag.  I
tried to write a function which would detect when two types came from
different translation units, but it doesn't work: --enable-intermodule
bootstrap blows up with tons of spurious duplicate declarations.

This is the code I wrote:-

/* Determine whether two types derive from the same translation unit.
   If the CONTEXT chain ends in a null, that type's context is still
   being parsed, so if two types have context chains ending in null,
   they're in the same translation unit.  */
static int
same_translation_unit_p (tree t1, tree t2)
{
  while (t1 && TREE_CODE (t1) != TRANSLATION_UNIT_DECL)
    switch (TREE_CODE_CLASS (TREE_CODE (t1)))
      {
      case 'd': t1 = DECL_CONTEXT (t1); break;
      case 't': t1 = TYPE_CONTEXT (t1); break;
      case 'b': t1 = BLOCK_SUPERCONTEXT (t1); break;
      default: abort ();
      }

  while (t2 && TREE_CODE (t2) != TRANSLATION_UNIT_DECL)
    switch (TREE_CODE_CLASS (TREE_CODE (t2)))
      {
      case 'd': t2 = DECL_CONTEXT (t1); break;
      case 't': t2 = TYPE_CONTEXT (t2); break;
      case 'b': t2 = BLOCK_SUPERCONTEXT (t2); break;
      default: abort ();
      }


  return t1 == t2;
}

This replaces the flags check guarding the call to
tagged_types_tu_compatible_p.  What am I doing wrong?

zw

Re: intermodule and comptypes - need some help

[Geoff Keating]

"Zack Weinberg" <zack@codesourcery.com> writes:

> A patch I'm working on has, as a side effect, that (in intermodule
> mode) duplicate_decls may encounter two declarations from different
> translation units at any time, not just during
> merge_translation_unit_decls.

I'd be interested to hear more details about how this happens.

>  It therefore cannot simply be told when
> to invoke comptypes with the special COMPARE_DIFFERENT_TU flag.  I
> tried to write a function which would detect when two types came from
> different translation units, but it doesn't work: --enable-intermodule
> bootstrap blows up with tons of spurious duplicate declarations.
> 
> This is the code I wrote:-
> 
> /* Determine whether two types derive from the same translation unit.
>    If the CONTEXT chain ends in a null, that type's context is still
>    being parsed, so if two types have context chains ending in null,
>    they're in the same translation unit.  */
> static int
> same_translation_unit_p (tree t1, tree t2)
> {
>   while (t1 && TREE_CODE (t1) != TRANSLATION_UNIT_DECL)
>     switch (TREE_CODE_CLASS (TREE_CODE (t1)))
>       {
>       case 'd': t1 = DECL_CONTEXT (t1); break;
>       case 't': t1 = TYPE_CONTEXT (t1); break;
>       case 'b': t1 = BLOCK_SUPERCONTEXT (t1); break;
>       default: abort ();
>       }
> 
>   while (t2 && TREE_CODE (t2) != TRANSLATION_UNIT_DECL)
>     switch (TREE_CODE_CLASS (TREE_CODE (t2)))
>       {
>       case 'd': t2 = DECL_CONTEXT (t1); break;
>       case 't': t2 = TYPE_CONTEXT (t2); break;
>       case 'b': t2 = BLOCK_SUPERCONTEXT (t2); break;
>       default: abort ();
>       }
> 
> 
>   return t1 == t2;
> }
> 
> This replaces the flags check guarding the call to
> tagged_types_tu_compatible_p.  What am I doing wrong?

I expect that at some point, some type gets created without the right
context.  This would never have mattered before because nothing ever
looked at the context of types (except in the objc compiler and then
only in pretty special circumstances).

If bootstrapping GCC is getting to be a pain, you might find the
attached testcase helpful; you can modify it by assigning between x_1
and x_2 in the different translation units, the assignment is valid in
-6b but invalid (and requires a diagnostic) in -6a, and yet as written
the two files are valid ISO C.  Build it with

gcc -O3 imi-6a.c imi-6b.c -c -o imi-6.o
gcc imi-6.o -o imi-6
./imi-6

-- 
- Geoffrey Keating <geoffk@geoffk.org>

====================imi-6a.c
/* { dg-do run } */
/* { dg-options "-O3" } */

/* An obscure aliasing testcase: In this file, the types of 'x_1' and
   'x_2' and 'x' are not compatible.  But in the other file, they are
   all compatible.  */

extern struct 
{
  int x;
} *x_1;

extern struct 
{
  int x;
} *x_2;

extern struct 
{
  int x;
} x;

void foo(void)
{
  x.x = 4;
  x_2->x = 3;
  if (x.x != 3)
    abort ();
  x_1->x = 1;
  if (x_2->x != 1)
    abort ();
  if ((void *)x_1 != (void *)x_2)
    abort ();
  if ((void *)x_1 != &x)
    abort ();
}
====================
====================imi-6b.c
struct 
{
  int x;
} x, *x_1 = &x, *x_2 = &x;

extern void foo(void);

int main(void)
{
  foo();
}
====================

Re: intermodule and comptypes - need some help

[Zack Weinberg]

Geoff Keating <geoffk@geoffk.org> writes:

> "Zack Weinberg" <zack@codesourcery.com> writes:
>
>> A patch I'm working on has, as a side effect, that (in intermodule
>> mode) duplicate_decls may encounter two declarations from different
>> translation units at any time, not just during
>> merge_translation_unit_decls.
>
> I'd be interested to hear more details about how this happens.

The basic concept is, there's a scope (currently called
external_scope) which holds all TREE_PUBLIC declarations and possibly
some other things.  This scope is 'outer' to the file scope, and
persists across translation units.  Declarations in there are
invisible until a duplicate declaration of the same thing is
encountered.

The details are complicated, but I think the result winds up being
more straightforward and less buggy than what we have now.  If, that
is, I can make it work.

> I expect that at some point, some type gets created without the right
> context.  This would never have mattered before because nothing ever
> looked at the context of types (except in the objc compiler and then
> only in pretty special circumstances).

Thanks for the hint.  The predicate I wrote *should* work, then?

> If bootstrapping GCC is getting to be a pain, you might find the
> attached testcase helpful; you can modify it by assigning between x_1
> and x_2 in the different translation units, the assignment is valid in
> -6b but invalid (and requires a diagnostic) in -6a, and yet as written
> the two files are valid ISO C.

This is helpful, thanks, but it doesn't tickle the problem I am
seeing.  The problem is like this:

debug.h:127: error: conflicting types for 'dwarf_debug_hooks'
dwarfout.c:1275: error: previous declaration of 'dwarf_debug_hooks'

and, using your hint, I think I've tracked it to BLOCK_SUPERCONTEXT of
the block that's DECL_INITIAL of each translation_unit_decl not
getting initialized properly.  The types being handed to my predicate
*seem* to have the right TYPE_CONTEXTs.

zw

Re: intermodule and comptypes - need some help

[Geoff Keating]

> X-Original-To: geoffk@foam.wonderslug.com
> From: "Zack Weinberg" <zack@codesourcery.com>
> Cc: gcc@gcc.gnu.org
> Date: Thu, 07 Aug 2003 13:59:52 -0700
> User-Agent: Gnus/5.1002 (Gnus v5.10.2) Emacs/21.3 (gnu/linux)
> X-OriginalArrivalTime: 07 Aug 2003 21:04:02.0937 (UTC) FILETIME=[6E228690:01C35D27]
> 
> Geoff Keating <geoffk@geoffk.org> writes:
> 
> > "Zack Weinberg" <zack@codesourcery.com> writes:
> >
> >> A patch I'm working on has, as a side effect, that (in intermodule
> >> mode) duplicate_decls may encounter two declarations from different
> >> translation units at any time, not just during
> >> merge_translation_unit_decls.
> >
> > I'd be interested to hear more details about how this happens.
> 
> The basic concept is, there's a scope (currently called
> external_scope) which holds all TREE_PUBLIC declarations and possibly
> some other things.  This scope is 'outer' to the file scope, and
> persists across translation units.  Declarations in there are
> invisible until a duplicate declaration of the same thing is
> encountered.
> 
> The details are complicated, but I think the result winds up being
> more straightforward and less buggy than what we have now.  If, that
> is, I can make it work.

I considered that, but stopped when I discovered

extern int x;
static int x = 2;

We might want to consider banning that (it does produce a warning
now), but I suspect someone relies on it.

There's another case that you'd want to think about.  Consider this:

====================t1.c
struct foo;
extern void x (struct foo *);
struct foo {
  int a, b;
};
====================t2.c
struct foo {
  double d;
};
void x (struct foo *y)
{
  y->d = 0;
}
====================

This is invalid code, I think, but whether it is or is not it does make
the implementation tricky: if it's valid, you want to keep information
about 't1.c::struct foo' out of t2.c, and if it's invalid you want to
detect the error even if t2.c is compiled first.

> > I expect that at some point, some type gets created without the right
> > context.  This would never have mattered before because nothing ever
> > looked at the context of types (except in the objc compiler and then
> > only in pretty special circumstances).
> 
> Thanks for the hint.  The predicate I wrote *should* work, then?

I think it would be a good thing if we arranged for it to work, yes.

> > If bootstrapping GCC is getting to be a pain, you might find the
> > attached testcase helpful; you can modify it by assigning between x_1
> > and x_2 in the different translation units, the assignment is valid in
> > -6b but invalid (and requires a diagnostic) in -6a, and yet as written
> > the two files are valid ISO C.
> 
> This is helpful, thanks, but it doesn't tickle the problem I am
> seeing.  The problem is like this:
> 
> debug.h:127: error: conflicting types for 'dwarf_debug_hooks'
> dwarfout.c:1275: error: previous declaration of 'dwarf_debug_hooks'
> 
> and, using your hint, I think I've tracked it to BLOCK_SUPERCONTEXT of
> the block that's DECL_INITIAL of each translation_unit_decl not
> getting initialized properly.  The types being handed to my predicate
> *seem* to have the right TYPE_CONTEXTs.

Is there a mistake in that paragraph?  I didn't think
translation_unit_decls have DECL_INITIAL...

-- 
- Geoffrey Keating <geoffk@geoffk.org>

Re: intermodule and comptypes - need some help

[Zack Weinberg]

Geoff Keating <geoffk@geoffk.org> writes:

>> The basic concept is, there's a scope (currently called
>> external_scope) which holds all TREE_PUBLIC declarations and possibly
>> some other things.  This scope is 'outer' to the file scope, and
>> persists across translation units.  Declarations in there are
>> invisible until a duplicate declaration of the same thing is
>> encountered.
>> 
>> The details are complicated, but I think the result winds up being
>> more straightforward and less buggy than what we have now.  If, that
>> is, I can make it work.
>
> I considered that, but stopped when I discovered
>
> extern int x;
> static int x = 2;
>
> We might want to consider banning that (it does produce a warning
> now), but I suspect someone relies on it.

This is easy to handle - whether or not we want to ban it.  (At
present I am producing an unconditional warning, no more.)  The
trick is that visibility is decoupled from presence in the ->names
list of each scope.

If these appear in two different translation units (but both
at file scope) the static declaration will simply shadow the
external declaration - probably don't even want to generate a
-Wshadow warning.

If they appear in the same translation unit, then the external
declaration is put into the externals scope, but made visible 
in the file scope.  When processing the static declaration,
pushdecl notices the external decl already visible, issues a
warning, and shadows it with the static.

> ====================t1.c
> struct foo;
> extern void x (struct foo *);
> struct foo {
>   int a, b;
> };
> ====================t2.c
> struct foo {
>   double d;
> };
> void x (struct foo *y)
> {
>   y->d = 0;
> }
> ====================
>
> This is invalid code, I think, but whether it is or is not it does make
> the implementation tricky: if it's valid, you want to keep information
> about 't1.c::struct foo' out of t2.c, and if it's invalid you want to
> detect the error even if t2.c is compiled first.

Hm.  I think it's just undefined behavior if foo were to be called
from t1.c with that definition of struct foo visible.  We do _not_
want to issue diagnostics if x in t1 were static, for instance.

I'll think about this a bit more.

>> Thanks for the hint.  The predicate I wrote *should* work, then?
>
> I think it would be a good thing if we arranged for it to work, yes.

Ok.

>> and, using your hint, I think I've tracked it to BLOCK_SUPERCONTEXT of
>> the block that's DECL_INITIAL of each translation_unit_decl not
>> getting initialized properly.  The types being handed to my predicate
>> *seem* to have the right TYPE_CONTEXTs.
>
> Is there a mistake in that paragraph?  I didn't think
> translation_unit_decls have DECL_INITIAL...

Didn't you *invent* TRANSLATION_UNIT_DECL?

This is what my c_reset_state looks like now:

   /* Pop the global scope.  */
   if (current_scope != global_scope)
       current_scope = global_scope;
   file_scope_decl = current_file_decl;
   DECL_INITIAL (file_scope_decl) = poplevel (1, 0, 0);
+  BLOCK_SUPERCONTEXT (DECL_INITIAL (file_scope_decl)) = file_scope_decl;
   truly_local_externals = NULL_TREE;

Adding that line makes me able to get through an --enable-intermodule,
C only bootstrap.  I'll post the patch shortly.

zw

Re: intermodule and comptypes - need some help

[Geoff Keating]

> From: "Zack Weinberg" <zack@codesourcery.com>
> Date: Thu, 07 Aug 2003 17:48:49 -0700

> Geoff Keating <geoffk@geoffk.org> writes:
> 
> >> The basic concept is, there's a scope (currently called
> >> external_scope) which holds all TREE_PUBLIC declarations and possibly
> >> some other things.  This scope is 'outer' to the file scope, and
> >> persists across translation units.  Declarations in there are
> >> invisible until a duplicate declaration of the same thing is
> >> encountered.
> >> 
> >> The details are complicated, but I think the result winds up being
> >> more straightforward and less buggy than what we have now.  If, that
> >> is, I can make it work.
> >
> > I considered that, but stopped when I discovered
> >
> > extern int x;
> > static int x = 2;
> >
> > We might want to consider banning that (it does produce a warning
> > now), but I suspect someone relies on it.
> 
> This is easy to handle - whether or not we want to ban it.  (At
> present I am producing an unconditional warning, no more.)  The
> trick is that visibility is decoupled from presence in the ->names
> list of each scope.
> 
> If these appear in two different translation units (but both
> at file scope) the static declaration will simply shadow the
> external declaration - probably don't even want to generate a
> -Wshadow warning.
> 
> If they appear in the same translation unit, then the external
> declaration is put into the externals scope, but made visible 
> in the file scope.  When processing the static declaration,
> pushdecl notices the external decl already visible, issues a
> warning, and shadows it with the static.

Note that this is a change of behaviour; before, this used to cause
'x' to refer to the static variable throughout the translation unit.
This would make a difference if it's 'extern const int'.

> > ====================t1.c
> > struct foo;
> > extern void x (struct foo *);
> > struct foo {
> >   int a, b;
> > };
> > ====================t2.c
> > struct foo {
> >   double d;
> > };
> > void x (struct foo *y)
> > {
> >   y->d = 0;
> > }
> > ====================
> >
> > This is invalid code, I think, but whether it is or is not it does make
> > the implementation tricky: if it's valid, you want to keep information
> > about 't1.c::struct foo' out of t2.c, and if it's invalid you want to
> > detect the error even if t2.c is compiled first.
> 
> Hm.  I think it's just undefined behavior if foo were to be called
> from t1.c with that definition of struct foo visible.  We do _not_
> want to issue diagnostics if x in t1 were static, for instance.

Yes, you're certainly allowed to have the same tag defined in two
different translation units with different values.  It's undefined
behaviour to declare the same object (as determined in this case by the
linkage rules) with incompatible types.  I am not, however, sure
whether or not this particular example counts as a declaration with
incompatible types; at the end of the translation unit, they are
incompatible, but not at the point of the declaration.

Yes, it's undefined behaviour if 'x' was actually called with that
definition of struct foo visible... although, again, I'm not sure
what happens if you call it when the structure is still incomplete.

> I'll think about this a bit more.

...

> >> and, using your hint, I think I've tracked it to BLOCK_SUPERCONTEXT of
> >> the block that's DECL_INITIAL of each translation_unit_decl not
> >> getting initialized properly.  The types being handed to my predicate
> >> *seem* to have the right TYPE_CONTEXTs.
> >
> > Is there a mistake in that paragraph?  I didn't think
> > translation_unit_decls have DECL_INITIAL...
> 
> Didn't you *invent* TRANSLATION_UNIT_DECL?
> 
> This is what my c_reset_state looks like now:
> 
>    /* Pop the global scope.  */
>    if (current_scope != global_scope)
>        current_scope = global_scope;
>    file_scope_decl = current_file_decl;
>    DECL_INITIAL (file_scope_decl) = poplevel (1, 0, 0);
> +  BLOCK_SUPERCONTEXT (DECL_INITIAL (file_scope_decl)) = file_scope_decl;
>    truly_local_externals = NULL_TREE;
> 
> Adding that line makes me able to get through an --enable-intermodule,
> C only bootstrap.  I'll post the patch shortly.

Oh, I remember now.  Yes, that seems right.

-- 
- Geoffrey Keating <geoffk@geoffk.org>