| Message ID | alpine.LSU.2.20.1903151430370.4934@zhemvz.fhfr.qr |
|---|---|
| State | New |
| Headers | show |
| Series | Fix PR71598, aliasing between enums and compatible types | expand |
Hi, On Fri, 15 Mar 2019, Richard Biener wrote: > The following is an attempt to fix PR71598 where C (and C++?) have an > implementation-defined compatible integer type for each enum and the > TBAA rules mandate that accesses using a compatible type are allowed. But different enums aren't compatible with each other (or are they?), while your fix simply gives enums the aliasing set of the underlying type, i.e. makes all of them alias with each other. That's quite conservative. Ciao, Michael. > > The fix is applied to all C family frontends and the LTO frontend > but not Fortran, Ada or other languages. > > Bootstrap & regtest running on x86_64-unknown-linux-gnu. > > I've tried to cover most cases even those with -fshort-enums. > > OK for trunk? > > It's probably a regression to some ancient GCC that didn't > perform TBAA and given it's wrong-code a backport is probably > mandated - do you agree? (after a while with no reported issues, > of course) > > Thanks, > Richard. > > 2019-03-15 Richard Biener <rguenther@suse.de> > > PR c/71598 > * gimple.c: Include langhooks.h. > (gimple_get_alias_set): Treat enumeral types as the underlying > integer type. > > c-family/ > * c-common.c (c_common_get_alias_set): Treat enumeral types > as the underlying integer type. > > * c-c++-common/torture/pr71598-1.c: New testcase. > * c-c++-common/torture/pr71598-2.c: Likewise. > > Index: gcc/gimple.c > =================================================================== > --- gcc/gimple.c (revision 269704) > +++ gcc/gimple.c (working copy) > @@ -44,6 +44,7 @@ along with GCC; see the file COPYING3. > #include "stringpool.h" > #include "attribs.h" > #include "asan.h" > +#include "langhooks.h" > > > /* All the tuples have their operand vector (if present) at the very bottom > @@ -2587,6 +2588,16 @@ gimple_get_alias_set (tree t) > return get_alias_set (t1); > } > > + /* Allow aliasing between enumeral types and the underlying > + integer type. This is required for C since those are > + compatible types. */ > + else if (TREE_CODE (t) == ENUMERAL_TYPE) > + { > + tree t1 = lang_hooks.types.type_for_size (tree_to_uhwi (TYPE_SIZE (t)), > + false /* short-cut above */); > + return get_alias_set (t1); > + } > + > return -1; > } > > Index: gcc/c-family/c-common.c > =================================================================== > --- gcc/c-family/c-common.c (revision 269704) > +++ gcc/c-family/c-common.c (working copy) > @@ -3681,6 +3681,15 @@ c_common_get_alias_set (tree t) > return get_alias_set (t1); > } > > + /* Allow aliasing between enumeral types and the underlying > + integer type. This is required since those are compatible types. */ > + else if (TREE_CODE (t) == ENUMERAL_TYPE) > + { > + tree t1 = c_common_type_for_size (tree_to_uhwi (TYPE_SIZE (t)), > + false /* short-cut above */); > + return get_alias_set (t1); > + } > + > return -1; > } > > Index: gcc/testsuite/c-c++-common/torture/pr71598-1.c > =================================================================== > --- gcc/testsuite/c-c++-common/torture/pr71598-1.c (nonexistent) > +++ gcc/testsuite/c-c++-common/torture/pr71598-1.c (working copy) > @@ -0,0 +1,21 @@ > +/* { dg-do run } */ > +/* { dg-additional-options "-fno-short-enums" } */ > + > +enum e1 { c1 }; > + > +__attribute__((noinline,noclone)) > +int f(enum e1 *p, unsigned *q) > +{ > + *p = c1; > + *q = 2; > + return *p; > +} > + > +int main() > +{ > + unsigned x; > + > + if (f((enum e1 *)&x, &x) != 2) > + __builtin_abort(); > + return 0; > +} > Index: gcc/testsuite/c-c++-common/torture/pr71598-2.c > =================================================================== > --- gcc/testsuite/c-c++-common/torture/pr71598-2.c (nonexistent) > +++ gcc/testsuite/c-c++-common/torture/pr71598-2.c (working copy) > @@ -0,0 +1,47 @@ > +/* { dg-do run } */ > +/* { dg-additional-options "-fshort-enums" } */ > + > +enum e1 { c1 = -__INT_MAX__ }; > + > +__attribute__((noinline,noclone)) > +int f(enum e1 *p, signed int *q) > +{ > + *p = c1; > + *q = 2; > + return *p; > +} > + > +enum e2 { c2 = __SHRT_MAX__ + 1}; > + > +__attribute__((noinline,noclone)) > +int g(enum e2 *p, unsigned short *q) > +{ > + *p = c2; > + *q = 2; > + return *p; > +} > + > +enum e3 { c3 = __SCHAR_MAX__ }; > + > +__attribute__((noinline,noclone)) > +int h(enum e3 *p, unsigned char *q) > +{ > + *p = c3; > + *q = 2; > + return *p; > +} > + > +int main() > +{ > + signed x; > + unsigned short y; > + unsigned char z; > + > + if (f((enum e1 *)&x, &x) != 2) > + __builtin_abort(); > + if (g((enum e2 *)&y, &y) != 2) > + __builtin_abort(); > + if (h((enum e3 *)&z, &z) != 2) > + __builtin_abort(); > + return 0; > +} >
On March 15, 2019 3:39:16 PM GMT+01:00, Michael Matz <matz@suse.de> wrote: >Hi, > >On Fri, 15 Mar 2019, Richard Biener wrote: > >> The following is an attempt to fix PR71598 where C (and C++?) have an > >> implementation-defined compatible integer type for each enum and the >> TBAA rules mandate that accesses using a compatible type are allowed. > >But different enums aren't compatible with each other (or are they?), >while your fix simply gives enums the aliasing set of the underlying >type, >i.e. makes all of them alias with each other. That's quite >conservative. But that follows from the need to be able to access an enum as int and the other way around. So both alias sets need to be subsets of each other which means they need to be equal. Richard. > >Ciao, >Michael. > >> >> The fix is applied to all C family frontends and the LTO frontend >> but not Fortran, Ada or other languages. >> >> Bootstrap & regtest running on x86_64-unknown-linux-gnu. >> >> I've tried to cover most cases even those with -fshort-enums. >> >> OK for trunk? >> >> It's probably a regression to some ancient GCC that didn't >> perform TBAA and given it's wrong-code a backport is probably >> mandated - do you agree? (after a while with no reported issues, >> of course) >> >> Thanks, >> Richard. >> >> 2019-03-15 Richard Biener <rguenther@suse.de> >> >> PR c/71598 >> * gimple.c: Include langhooks.h. >> (gimple_get_alias_set): Treat enumeral types as the underlying >> integer type. >> >> c-family/ >> * c-common.c (c_common_get_alias_set): Treat enumeral types >> as the underlying integer type. >> >> * c-c++-common/torture/pr71598-1.c: New testcase. >> * c-c++-common/torture/pr71598-2.c: Likewise. >> >> Index: gcc/gimple.c >> =================================================================== >> --- gcc/gimple.c (revision 269704) >> +++ gcc/gimple.c (working copy) >> @@ -44,6 +44,7 @@ along with GCC; see the file COPYING3. >> #include "stringpool.h" >> #include "attribs.h" >> #include "asan.h" >> +#include "langhooks.h" >> >> >> /* All the tuples have their operand vector (if present) at the very >bottom >> @@ -2587,6 +2588,16 @@ gimple_get_alias_set (tree t) >> return get_alias_set (t1); >> } >> >> + /* Allow aliasing between enumeral types and the underlying >> + integer type. This is required for C since those are >> + compatible types. */ >> + else if (TREE_CODE (t) == ENUMERAL_TYPE) >> + { >> + tree t1 = lang_hooks.types.type_for_size (tree_to_uhwi >(TYPE_SIZE (t)), >> + false /* short-cut above */); >> + return get_alias_set (t1); >> + } >> + >> return -1; >> } >> >> Index: gcc/c-family/c-common.c >> =================================================================== >> --- gcc/c-family/c-common.c (revision 269704) >> +++ gcc/c-family/c-common.c (working copy) >> @@ -3681,6 +3681,15 @@ c_common_get_alias_set (tree t) >> return get_alias_set (t1); >> } >> >> + /* Allow aliasing between enumeral types and the underlying >> + integer type. This is required since those are compatible >types. */ >> + else if (TREE_CODE (t) == ENUMERAL_TYPE) >> + { >> + tree t1 = c_common_type_for_size (tree_to_uhwi (TYPE_SIZE >(t)), >> + false /* short-cut above */); >> + return get_alias_set (t1); >> + } >> + >> return -1; >> } >> > >> Index: gcc/testsuite/c-c++-common/torture/pr71598-1.c >> =================================================================== >> --- gcc/testsuite/c-c++-common/torture/pr71598-1.c (nonexistent) >> +++ gcc/testsuite/c-c++-common/torture/pr71598-1.c (working copy) >> @@ -0,0 +1,21 @@ >> +/* { dg-do run } */ >> +/* { dg-additional-options "-fno-short-enums" } */ >> + >> +enum e1 { c1 }; >> + >> +__attribute__((noinline,noclone)) >> +int f(enum e1 *p, unsigned *q) >> +{ >> + *p = c1; >> + *q = 2; >> + return *p; >> +} >> + >> +int main() >> +{ >> + unsigned x; >> + >> + if (f((enum e1 *)&x, &x) != 2) >> + __builtin_abort(); >> + return 0; >> +} >> Index: gcc/testsuite/c-c++-common/torture/pr71598-2.c >> =================================================================== >> --- gcc/testsuite/c-c++-common/torture/pr71598-2.c (nonexistent) >> +++ gcc/testsuite/c-c++-common/torture/pr71598-2.c (working copy) >> @@ -0,0 +1,47 @@ >> +/* { dg-do run } */ >> +/* { dg-additional-options "-fshort-enums" } */ >> + >> +enum e1 { c1 = -__INT_MAX__ }; >> + >> +__attribute__((noinline,noclone)) >> +int f(enum e1 *p, signed int *q) >> +{ >> + *p = c1; >> + *q = 2; >> + return *p; >> +} >> + >> +enum e2 { c2 = __SHRT_MAX__ + 1}; >> + >> +__attribute__((noinline,noclone)) >> +int g(enum e2 *p, unsigned short *q) >> +{ >> + *p = c2; >> + *q = 2; >> + return *p; >> +} >> + >> +enum e3 { c3 = __SCHAR_MAX__ }; >> + >> +__attribute__((noinline,noclone)) >> +int h(enum e3 *p, unsigned char *q) >> +{ >> + *p = c3; >> + *q = 2; >> + return *p; >> +} >> + >> +int main() >> +{ >> + signed x; >> + unsigned short y; >> + unsigned char z; >> + >> + if (f((enum e1 *)&x, &x) != 2) >> + __builtin_abort(); >> + if (g((enum e2 *)&y, &y) != 2) >> + __builtin_abort(); >> + if (h((enum e3 *)&z, &z) != 2) >> + __builtin_abort(); >> + return 0; >> +} >>
Hi, On Fri, 15 Mar 2019, Richard Biener wrote: > >But different enums aren't compatible with each other (or are they?), > >while your fix simply gives enums the aliasing set of the underlying > >type, i.e. makes all of them alias with each other. That's quite > >conservative. > > But that follows from the need to be able to access an enum as int and > the other way around. So both alias sets need to be subsets of each > other which means they need to be equal. No. Suppose you have enum E1, E2 and int, you have to arrange it such that aliasset(E1) \subset aliasset(int) && aliasset(E2) \subset aliasset(int). That doesn't (and shouldn't) imply any relationship between aliassets of E1 and E2. Of course you can only express something else than equality when you aren't making all three of them the same set-identifiers. I.e. what you touched is the naming of sets (giving them identifiers), whereas you should have touched where the relations between the sets are established. I _think_ instead of giving enum and basetypes the same alias set you should have rather called record_alias_subset(intset, enumset) (or the other way around, I'm always confused there :) ). Ciao, Michael.
Hi, On Fri, 15 Mar 2019, Michael Matz wrote: > I.e. what you touched is the naming of sets (giving them identifiers), > whereas you should have touched where the relations between the sets are > established. I _think_ instead of giving enum and basetypes the same > alias set you should have rather called record_alias_subset(intset, > enumset) (or the other way around, I'm always confused there :) ). Or, because an enum with these properties could be modelled as a struct containing one member of basetype you could also change record_component_aliases(), though that doesn't allow language specific behaviour differences. Ciao, Michael.
On 3/15/19 9:33 AM, Richard Biener wrote: > > The following is an attempt to fix PR71598 where C (and C++?) have > an implementation-defined compatible integer type for each enum > and the TBAA rules mandate that accesses using a compatible type > are allowed. This does not apply to C++; an enum does not alias its underlying type. Jason
On Fri, 15 Mar 2019, Michael Matz wrote: > Hi, > > On Fri, 15 Mar 2019, Michael Matz wrote: > > > I.e. what you touched is the naming of sets (giving them identifiers), > > whereas you should have touched where the relations between the sets are > > established. I _think_ instead of giving enum and basetypes the same > > alias set you should have rather called record_alias_subset(intset, > > enumset) (or the other way around, I'm always confused there :) ). > > Or, because an enum with these properties could be modelled as a struct > containing one member of basetype you could also change > record_component_aliases(), though that doesn't allow language specific > behaviour differences. No, you can't. I believe that enum X and int being compatible means that accessing an enum X object via an lvalue of effective type int is OK _and_ accessing an int object via an lvalue of effective type enum X is OK. That commutativity doesn't hold for struct X { int i; } vs. int i and those types are not compatible. At least unless Joseph corrects me here and that "type X is compatible with type Y" doesn't imply "type Y is compatible with type X" (that's not transitivity). Now, we can't currently model precisely this way of non-transitivity of C's notion of "compatibility". enum X { A }; enum Y { B }; void *ptr = malloc (4); *(enum X *)ptr = A; // dynamic type is now X foo (*(enum Y *)ptr); // undefined, X and Y are not compatible(?) foo (*(int *)ptr); // OK, X and int are compatible *(int *)ptr = *(enum X *); // dynamic type is now int foo (*(enum Y *)ptr); // OK(?), Y and int are compatible Joseph, do you agree that enum and int are to be handled the same way as we handle unsigned vs. signed integer types (though those get an extra bullet in 6.5/7 and so they are probably not compatible). Richard.
On Fri, 15 Mar 2019, Jason Merrill wrote: > On 3/15/19 9:33 AM, Richard Biener wrote: > > > > The following is an attempt to fix PR71598 where C (and C++?) have > > an implementation-defined compatible integer type for each enum > > and the TBAA rules mandate that accesses using a compatible type > > are allowed. > > This does not apply to C++; an enum does not alias its underlying type. Thus the following different patch, introducing c_get_alias_set and only doing the special handling for C family frontends (I assume that at least ObjC is also affected). Bootstrap & regtest running on x86_64-unknown-linux-gnu, OK? Thanks, Richard. 2019-03-18 Richard Biener <rguenther@suse.de> PR c/71598 * gimple.c: Include langhooks.h. (gimple_get_alias_set): Treat enumeral types as the underlying integer type. c/ * c-tree.h (c_get_alias_set): Declare. * c-objc-common.h (LANG_HOOKS_GET_ALIAS_SET): Use c_get_alias_set. * c-objc-common.c (c_get_alias_set): Treat enumeral types as the underlying integer type. * gcc.dg/torture/pr71598-1.c: New testcase. * gcc.dg/torture/pr71598-2.c: Likewise. Index: gcc/c/c-objc-common.c =================================================================== --- gcc/c/c-objc-common.c (revision 269752) +++ gcc/c/c-objc-common.c (working copy) @@ -265,3 +265,22 @@ c_vla_unspec_p (tree x, tree fn ATTRIBUT { return c_vla_type_p (x); } + +/* Special routine to get the alias set of T for C. */ + +alias_set_type +c_get_alias_set (tree t) +{ + /* Allow aliasing between enumeral types and the underlying + integer type. This is required since those are compatible types. */ + if (TREE_CODE (t) == ENUMERAL_TYPE) + { + tree t1 = c_common_type_for_size (tree_to_uhwi (TYPE_SIZE (t)), + /* short-cut commoning to signed + type. */ + false); + return get_alias_set (t1); + } + + return c_common_get_alias_set (t); +} Index: gcc/c/c-objc-common.h =================================================================== --- gcc/c/c-objc-common.h (revision 269752) +++ gcc/c/c-objc-common.h (working copy) @@ -43,7 +43,7 @@ along with GCC; see the file COPYING3. #undef LANG_HOOKS_POST_OPTIONS #define LANG_HOOKS_POST_OPTIONS c_common_post_options #undef LANG_HOOKS_GET_ALIAS_SET -#define LANG_HOOKS_GET_ALIAS_SET c_common_get_alias_set +#define LANG_HOOKS_GET_ALIAS_SET c_get_alias_set #undef LANG_HOOKS_PARSE_FILE #define LANG_HOOKS_PARSE_FILE c_common_parse_file #undef LANG_HOOKS_FINISH_INCOMPLETE_DECL Index: gcc/c/c-tree.h =================================================================== --- gcc/c/c-tree.h (revision 269752) +++ gcc/c/c-tree.h (working copy) @@ -623,6 +623,7 @@ extern bool c_missing_noreturn_ok_p (tre extern bool c_warn_unused_global_decl (const_tree); extern void c_initialize_diagnostics (diagnostic_context *); extern bool c_vla_unspec_p (tree x, tree fn); +extern alias_set_type c_get_alias_set (tree); /* in c-typeck.c */ extern int in_alignof; Index: gcc/gimple.c =================================================================== --- gcc/gimple.c (revision 269752) +++ gcc/gimple.c (working copy) @@ -44,6 +44,7 @@ along with GCC; see the file COPYING3. #include "stringpool.h" #include "attribs.h" #include "asan.h" +#include "langhooks.h" /* All the tuples have their operand vector (if present) at the very bottom @@ -2587,6 +2588,16 @@ gimple_get_alias_set (tree t) return get_alias_set (t1); } + /* Allow aliasing between enumeral types and the underlying + integer type. This is required for C since those are + compatible types. */ + else if (TREE_CODE (t) == ENUMERAL_TYPE) + { + tree t1 = lang_hooks.types.type_for_size (tree_to_uhwi (TYPE_SIZE (t)), + false /* short-cut above */); + return get_alias_set (t1); + } + return -1; } Index: gcc/testsuite/gcc.dg/torture/pr71598-1.c =================================================================== --- gcc/testsuite/gcc.dg/torture/pr71598-1.c (nonexistent) +++ gcc/testsuite/gcc.dg/torture/pr71598-1.c (working copy) @@ -0,0 +1,21 @@ +/* { dg-do run } */ +/* { dg-additional-options "-fno-short-enums" } */ + +enum e1 { c1 }; + +__attribute__((noinline,noclone)) +int f(enum e1 *p, unsigned *q) +{ + *p = c1; + *q = 2; + return *p; +} + +int main() +{ + unsigned x; + + if (f(&x, &x) != 2) + __builtin_abort(); + return 0; +} Index: gcc/testsuite/gcc.dg/torture/pr71598-2.c =================================================================== --- gcc/testsuite/gcc.dg/torture/pr71598-2.c (nonexistent) +++ gcc/testsuite/gcc.dg/torture/pr71598-2.c (working copy) @@ -0,0 +1,47 @@ +/* { dg-do run } */ +/* { dg-additional-options "-fshort-enums" } */ + +enum e1 { c1 = -__INT_MAX__ }; + +__attribute__((noinline,noclone)) +int f(enum e1 *p, signed int *q) +{ + *p = c1; + *q = 2; + return *p; +} + +enum e2 { c2 = __SHRT_MAX__ + 1}; + +__attribute__((noinline,noclone)) +int g(enum e2 *p, unsigned short *q) +{ + *p = c2; + *q = 2; + return *p; +} + +enum e3 { c3 = __SCHAR_MAX__ }; + +__attribute__((noinline,noclone)) +int h(enum e3 *p, unsigned char *q) +{ + *p = c3; + *q = 2; + return *p; +} + +int main() +{ + signed x; + unsigned short y; + unsigned char z; + + if (f(&x, &x) != 2) + __builtin_abort(); + if (g(&y, &y) != 2) + __builtin_abort(); + if (h(&z, &z) != 2) + __builtin_abort(); + return 0; +}
> On 18 Mar 2019, at 09:12, Richard Biener <rguenther@suse.de> wrote: > > On Fri, 15 Mar 2019, Jason Merrill wrote: > >> On 3/15/19 9:33 AM, Richard Biener wrote: >>> >>> The following is an attempt to fix PR71598 where C (and C++?) have >>> an implementation-defined compatible integer type for each enum >>> and the TBAA rules mandate that accesses using a compatible type >>> are allowed. >> >> This does not apply to C++; an enum does not alias its underlying type. > > Thus the following different patch, introducing c_get_alias_set and > only doing the special handling for C family frontends (I assume > that at least ObjC is also affected). As far as ObjC is concerned, I’m not aware of any special rule, thus it should behave as per the underlying C impl (therefore if it’s OK for C it is OK for ObjC). Iain > Bootstrap & regtest running on x86_64-unknown-linux-gnu, OK? > > Thanks, > Richard. > > 2019-03-18 Richard Biener <rguenther@suse.de> > > PR c/71598 > * gimple.c: Include langhooks.h. > (gimple_get_alias_set): Treat enumeral types as the underlying > integer type. > > c/ > * c-tree.h (c_get_alias_set): Declare. > * c-objc-common.h (LANG_HOOKS_GET_ALIAS_SET): Use c_get_alias_set. > * c-objc-common.c (c_get_alias_set): Treat enumeral types > as the underlying integer type. > > * gcc.dg/torture/pr71598-1.c: New testcase. > * gcc.dg/torture/pr71598-2.c: Likewise. > > > Index: gcc/c/c-objc-common.c > =================================================================== > --- gcc/c/c-objc-common.c (revision 269752) > +++ gcc/c/c-objc-common.c (working copy) > @@ -265,3 +265,22 @@ c_vla_unspec_p (tree x, tree fn ATTRIBUT > { > return c_vla_type_p (x); > } > + > +/* Special routine to get the alias set of T for C. */ > + > +alias_set_type > +c_get_alias_set (tree t) > +{ > + /* Allow aliasing between enumeral types and the underlying > + integer type. This is required since those are compatible types. */ > + if (TREE_CODE (t) == ENUMERAL_TYPE) > + { > + tree t1 = c_common_type_for_size (tree_to_uhwi (TYPE_SIZE (t)), > + /* short-cut commoning to signed > + type. */ > + false); > + return get_alias_set (t1); > + } > + > + return c_common_get_alias_set (t); > +} > Index: gcc/c/c-objc-common.h > =================================================================== > --- gcc/c/c-objc-common.h (revision 269752) > +++ gcc/c/c-objc-common.h (working copy) > @@ -43,7 +43,7 @@ along with GCC; see the file COPYING3. > #undef LANG_HOOKS_POST_OPTIONS > #define LANG_HOOKS_POST_OPTIONS c_common_post_options > #undef LANG_HOOKS_GET_ALIAS_SET > -#define LANG_HOOKS_GET_ALIAS_SET c_common_get_alias_set > +#define LANG_HOOKS_GET_ALIAS_SET c_get_alias_set > #undef LANG_HOOKS_PARSE_FILE > #define LANG_HOOKS_PARSE_FILE c_common_parse_file > #undef LANG_HOOKS_FINISH_INCOMPLETE_DECL > Index: gcc/c/c-tree.h > =================================================================== > --- gcc/c/c-tree.h (revision 269752) > +++ gcc/c/c-tree.h (working copy) > @@ -623,6 +623,7 @@ extern bool c_missing_noreturn_ok_p (tre > extern bool c_warn_unused_global_decl (const_tree); > extern void c_initialize_diagnostics (diagnostic_context *); > extern bool c_vla_unspec_p (tree x, tree fn); > +extern alias_set_type c_get_alias_set (tree); > > /* in c-typeck.c */ > extern int in_alignof; > Index: gcc/gimple.c > =================================================================== > --- gcc/gimple.c (revision 269752) > +++ gcc/gimple.c (working copy) > @@ -44,6 +44,7 @@ along with GCC; see the file COPYING3. > #include "stringpool.h" > #include "attribs.h" > #include "asan.h" > +#include "langhooks.h" > > > /* All the tuples have their operand vector (if present) at the very bottom > @@ -2587,6 +2588,16 @@ gimple_get_alias_set (tree t) > return get_alias_set (t1); > } > > + /* Allow aliasing between enumeral types and the underlying > + integer type. This is required for C since those are > + compatible types. */ > + else if (TREE_CODE (t) == ENUMERAL_TYPE) > + { > + tree t1 = lang_hooks.types.type_for_size (tree_to_uhwi (TYPE_SIZE (t)), > + false /* short-cut above */); > + return get_alias_set (t1); > + } > + > return -1; > } > > Index: gcc/testsuite/gcc.dg/torture/pr71598-1.c > =================================================================== > --- gcc/testsuite/gcc.dg/torture/pr71598-1.c (nonexistent) > +++ gcc/testsuite/gcc.dg/torture/pr71598-1.c (working copy) > @@ -0,0 +1,21 @@ > +/* { dg-do run } */ > +/* { dg-additional-options "-fno-short-enums" } */ > + > +enum e1 { c1 }; > + > +__attribute__((noinline,noclone)) > +int f(enum e1 *p, unsigned *q) > +{ > + *p = c1; > + *q = 2; > + return *p; > +} > + > +int main() > +{ > + unsigned x; > + > + if (f(&x, &x) != 2) > + __builtin_abort(); > + return 0; > +} > Index: gcc/testsuite/gcc.dg/torture/pr71598-2.c > =================================================================== > --- gcc/testsuite/gcc.dg/torture/pr71598-2.c (nonexistent) > +++ gcc/testsuite/gcc.dg/torture/pr71598-2.c (working copy) > @@ -0,0 +1,47 @@ > +/* { dg-do run } */ > +/* { dg-additional-options "-fshort-enums" } */ > + > +enum e1 { c1 = -__INT_MAX__ }; > + > +__attribute__((noinline,noclone)) > +int f(enum e1 *p, signed int *q) > +{ > + *p = c1; > + *q = 2; > + return *p; > +} > + > +enum e2 { c2 = __SHRT_MAX__ + 1}; > + > +__attribute__((noinline,noclone)) > +int g(enum e2 *p, unsigned short *q) > +{ > + *p = c2; > + *q = 2; > + return *p; > +} > + > +enum e3 { c3 = __SCHAR_MAX__ }; > + > +__attribute__((noinline,noclone)) > +int h(enum e3 *p, unsigned char *q) > +{ > + *p = c3; > + *q = 2; > + return *p; > +} > + > +int main() > +{ > + signed x; > + unsigned short y; > + unsigned char z; > + > + if (f(&x, &x) != 2) > + __builtin_abort(); > + if (g(&y, &y) != 2) > + __builtin_abort(); > + if (h(&z, &z) != 2) > + __builtin_abort(); > + return 0; > +}
Hi, On Mon, 18 Mar 2019, Richard Biener wrote: > > Or, because an enum with these properties could be modelled as a struct > > containing one member of basetype you could also change > > record_component_aliases(), though that doesn't allow language specific > > behaviour differences. > > No, you can't. I believe that enum X and int being compatible means > that accessing an enum X object via an lvalue of effective type int > is OK _and_ accessing an int object via an lvalue of effective type > enum X is OK. I agree. But the objects type still remains whatever it was: either an enum or an int; type equivalence/identity isn't affected by compatiblity. > That commutativity doesn't hold for struct X { int i; } > vs. int i and those types are not compatible. True, but that's not the important aspect that the aliasing subsetting expresses: if A subsets B or B subsets A, all accesses between an A and a B conflict. If A is the "struct S1 {int}" and B is "int" that's exactly right, an access to the struct and one to an int (_access_, where you don't necessarily know the declared type) conflict. But if we have a C, an "struct S2 {int}" the direction of the subsetting starts to matter: A conflict B, and C conflict B, but !(A conflict C). This is exactly the situation with A and C being different enums as well. The non-transitivity of the 'conflicts' relation is expressed by having a direction in the alias subset relation: everything that is (or contains) and A conflicts with everything that is (or contains) an int, everything that is (or contains) a C conflicts with int, but nothing that is (or contains) an A conflicts with anything that is (or contains) a C (if not for other reasons). > At least unless Joseph corrects me here and that "type X is compatible > with type Y" doesn't imply "type Y is compatible with type X" > (that's not transitivity). That's not transitivity but symmetry, and that of course holds for "compatible". X compat Y, and Z compat Y --> X compat Z would be transitivity and doesn't hold. > Now, we can't currently model precisely this way of non-transitivity > of C's notion of "compatibility". > > enum X { A }; > enum Y { B }; > void *ptr = malloc (4); > *(enum X *)ptr = A; // dynamic type is now X > foo (*(enum Y *)ptr); // undefined, X and Y are not compatible(?) > foo (*(int *)ptr); // OK, X and int are compatible > *(int *)ptr = *(enum X *); // dynamic type is now int > foo (*(enum Y *)ptr); // OK(?), Y and int are compatible I'm pretty sure this can be expressed, in the way I suggested, making X subset int, Y subset int (or superset? As said, I'm always confused), and those above would work. Ciao, Michael.
On Mon, 18 Mar 2019, Michael Matz wrote: > Hi, > > On Mon, 18 Mar 2019, Richard Biener wrote: > > > > Or, because an enum with these properties could be modelled as a struct > > > containing one member of basetype you could also change > > > record_component_aliases(), though that doesn't allow language specific > > > behaviour differences. > > > > No, you can't. I believe that enum X and int being compatible means > > that accessing an enum X object via an lvalue of effective type int > > is OK _and_ accessing an int object via an lvalue of effective type > > enum X is OK. > > I agree. But the objects type still remains whatever it was: either an > enum or an int; type equivalence/identity isn't affected by compatiblity. > > > That commutativity doesn't hold for struct X { int i; } > > vs. int i and those types are not compatible. > > True, but that's not the important aspect that the aliasing subsetting > expresses: if A subsets B or B subsets A, all accesses between an A and a > B conflict. If A is the "struct S1 {int}" and B is "int" that's exactly > right, an access to the struct and one to an int (_access_, where you > don't necessarily know the declared type) conflict. But if we have a C, > an "struct S2 {int}" the direction of the subsetting starts to matter: > A conflict B, and C conflict B, but !(A conflict C). This is exactly the > situation with A and C being different enums as well. > > The non-transitivity of the 'conflicts' relation is expressed by having a > direction in the alias subset relation: everything that is (or contains) > and A conflicts with everything that is (or contains) an int, everything > that is (or contains) a C conflicts with int, but nothing that is (or > contains) an A conflicts with anything that is (or contains) a C (if not > for other reasons). > > > At least unless Joseph corrects me here and that "type X is compatible > > with type Y" doesn't imply "type Y is compatible with type X" > > (that's not transitivity). > > That's not transitivity but symmetry, and that of course holds for > "compatible". X compat Y, and Z compat Y --> X compat Z would be > transitivity and doesn't hold. > > > Now, we can't currently model precisely this way of non-transitivity > > of C's notion of "compatibility". > > > > enum X { A }; > > enum Y { B }; > > void *ptr = malloc (4); > > *(enum X *)ptr = A; // dynamic type is now X > > foo (*(enum Y *)ptr); // undefined, X and Y are not compatible(?) > > foo (*(int *)ptr); // OK, X and int are compatible > > *(int *)ptr = *(enum X *); // dynamic type is now int > > foo (*(enum Y *)ptr); // OK(?), Y and int are compatible > > I'm pretty sure this can be expressed, in the way I suggested, making X > subset int, Y subset int (or superset? As said, I'm always confused), and > those above would work. It doesn't really. Consider the following IMHO valid testcase: enum X { A, B }; enum Y { C, D }; void foo (int *p) { *(enum X *)p = A; *(enum Y *)p = D; return *(enum X *)p; } int main() { int storage; if (foo (&storage) != A) abort (); } where in C a store doesn't change the dynamic type of an object with a declared type so the dynamic type stays 'int' always and thus both stores happen via compatible types. But the middle-end just sees stores via type X and Y which are not C-compatible. The middle-end doesn't know anything about those stores not updating the dynamic type which means any valid lvalue access according to 6.5/7 may not change the alias-set. So as I already said, our implementation cannot express this non-transitive compatibility in a safe manner. Richard.
Hi, On Tue, 19 Mar 2019, Richard Biener wrote: > It doesn't really. Consider the following IMHO valid testcase: > > enum X { A, B }; > enum Y { C, D }; > void foo (int *p) > { > *(enum X *)p = A; > *(enum Y *)p = D; > return *(enum X *)p; > } > > int main() > { > int storage; > if (foo (&storage) != A) (You want to require 'B' here, not 'A') > abort (); > } If you want the above testcase to be valid then yes, I agree, you have to give all enums the same alias set as the underlying type. Meh, I don't like it :-/ Ciao, Michael.
On Tue, 19 Mar 2019, Michael Matz wrote: > Hi, > > On Tue, 19 Mar 2019, Richard Biener wrote: > > > It doesn't really. Consider the following IMHO valid testcase: > > > > enum X { A, B }; > > enum Y { C, D }; > > void foo (int *p) > > { > > *(enum X *)p = A; > > *(enum Y *)p = D; > > return *(enum X *)p; > > } > > > > int main() > > { > > int storage; > > if (foo (&storage) != A) > > (You want to require 'B' here, not 'A') Eh, yes. Actually D I guess, or simply 1. > > abort (); > > } > > If you want the above testcase to be valid then yes, I agree, you have to > give all enums the same alias set as the underlying type. Meh, I don't > like it :-/ I think the C standard says the testcase should not abort. The patch still needs approval though. I'll make sure to add a testcase like the above. Richard.
On Mon, 18 Mar 2019, Richard Biener wrote: > On Fri, 15 Mar 2019, Jason Merrill wrote: > > > On 3/15/19 9:33 AM, Richard Biener wrote: > > > > > > The following is an attempt to fix PR71598 where C (and C++?) have > > > an implementation-defined compatible integer type for each enum > > > and the TBAA rules mandate that accesses using a compatible type > > > are allowed. > > > > This does not apply to C++; an enum does not alias its underlying type. > > Thus the following different patch, introducing c_get_alias_set and > only doing the special handling for C family frontends (I assume > that at least ObjC is also affected). > > Bootstrap & regtest running on x86_64-unknown-linux-gnu, OK? Ping. Also consider the additional testcase below to be added. Richard. 2019-03-18 Richard Biener <rguenther@suse.de> PR c/71598 * gimple.c: Include langhooks.h. (gimple_get_alias_set): Treat enumeral types as the underlying integer type. c/ * c-tree.h (c_get_alias_set): Declare. * c-objc-common.h (LANG_HOOKS_GET_ALIAS_SET): Use c_get_alias_set. * c-objc-common.c (c_get_alias_set): Treat enumeral types as the underlying integer type. * gcc.dg/torture/pr71598-1.c: New testcase. * gcc.dg/torture/pr71598-2.c: Likewise. * gcc.dg/torture/pr71598-3.c: Likewise. Index: gcc/testsuite/gcc.dg/torture/pr71598-3.c =================================================================== --- gcc/testsuite/gcc.dg/torture/pr71598-3.c (nonexistent) +++ gcc/testsuite/gcc.dg/torture/pr71598-3.c (working copy) @@ -0,0 +1,21 @@ +/* { dg-do run } */ + +enum e1 { A, B }; +enum e2 { C, D }; + +__attribute__((noinline,noclone)) +enum e1 f(unsigned int *p) +{ + *(enum e1 *)p = A; + *(enum e2 *)p = D; + return *(enum e1 *)p; +} + +int main() +{ + unsigned int storage; + + if (f(&storage) != B) + __builtin_abort(); + return 0; +} > Thanks, > Richard. > > 2019-03-18 Richard Biener <rguenther@suse.de> > > PR c/71598 > * gimple.c: Include langhooks.h. > (gimple_get_alias_set): Treat enumeral types as the underlying > integer type. > > c/ > * c-tree.h (c_get_alias_set): Declare. > * c-objc-common.h (LANG_HOOKS_GET_ALIAS_SET): Use c_get_alias_set. > * c-objc-common.c (c_get_alias_set): Treat enumeral types > as the underlying integer type. > > * gcc.dg/torture/pr71598-1.c: New testcase. > * gcc.dg/torture/pr71598-2.c: Likewise. > > > Index: gcc/c/c-objc-common.c > =================================================================== > --- gcc/c/c-objc-common.c (revision 269752) > +++ gcc/c/c-objc-common.c (working copy) > @@ -265,3 +265,22 @@ c_vla_unspec_p (tree x, tree fn ATTRIBUT > { > return c_vla_type_p (x); > } > + > +/* Special routine to get the alias set of T for C. */ > + > +alias_set_type > +c_get_alias_set (tree t) > +{ > + /* Allow aliasing between enumeral types and the underlying > + integer type. This is required since those are compatible types. */ > + if (TREE_CODE (t) == ENUMERAL_TYPE) > + { > + tree t1 = c_common_type_for_size (tree_to_uhwi (TYPE_SIZE (t)), > + /* short-cut commoning to signed > + type. */ > + false); > + return get_alias_set (t1); > + } > + > + return c_common_get_alias_set (t); > +} > Index: gcc/c/c-objc-common.h > =================================================================== > --- gcc/c/c-objc-common.h (revision 269752) > +++ gcc/c/c-objc-common.h (working copy) > @@ -43,7 +43,7 @@ along with GCC; see the file COPYING3. > #undef LANG_HOOKS_POST_OPTIONS > #define LANG_HOOKS_POST_OPTIONS c_common_post_options > #undef LANG_HOOKS_GET_ALIAS_SET > -#define LANG_HOOKS_GET_ALIAS_SET c_common_get_alias_set > +#define LANG_HOOKS_GET_ALIAS_SET c_get_alias_set > #undef LANG_HOOKS_PARSE_FILE > #define LANG_HOOKS_PARSE_FILE c_common_parse_file > #undef LANG_HOOKS_FINISH_INCOMPLETE_DECL > Index: gcc/c/c-tree.h > =================================================================== > --- gcc/c/c-tree.h (revision 269752) > +++ gcc/c/c-tree.h (working copy) > @@ -623,6 +623,7 @@ extern bool c_missing_noreturn_ok_p (tre > extern bool c_warn_unused_global_decl (const_tree); > extern void c_initialize_diagnostics (diagnostic_context *); > extern bool c_vla_unspec_p (tree x, tree fn); > +extern alias_set_type c_get_alias_set (tree); > > /* in c-typeck.c */ > extern int in_alignof; > Index: gcc/gimple.c > =================================================================== > --- gcc/gimple.c (revision 269752) > +++ gcc/gimple.c (working copy) > @@ -44,6 +44,7 @@ along with GCC; see the file COPYING3. > #include "stringpool.h" > #include "attribs.h" > #include "asan.h" > +#include "langhooks.h" > > > /* All the tuples have their operand vector (if present) at the very bottom > @@ -2587,6 +2588,16 @@ gimple_get_alias_set (tree t) > return get_alias_set (t1); > } > > + /* Allow aliasing between enumeral types and the underlying > + integer type. This is required for C since those are > + compatible types. */ > + else if (TREE_CODE (t) == ENUMERAL_TYPE) > + { > + tree t1 = lang_hooks.types.type_for_size (tree_to_uhwi (TYPE_SIZE (t)), > + false /* short-cut above */); > + return get_alias_set (t1); > + } > + > return -1; > } > > Index: gcc/testsuite/gcc.dg/torture/pr71598-1.c > =================================================================== > --- gcc/testsuite/gcc.dg/torture/pr71598-1.c (nonexistent) > +++ gcc/testsuite/gcc.dg/torture/pr71598-1.c (working copy) > @@ -0,0 +1,21 @@ > +/* { dg-do run } */ > +/* { dg-additional-options "-fno-short-enums" } */ > + > +enum e1 { c1 }; > + > +__attribute__((noinline,noclone)) > +int f(enum e1 *p, unsigned *q) > +{ > + *p = c1; > + *q = 2; > + return *p; > +} > + > +int main() > +{ > + unsigned x; > + > + if (f(&x, &x) != 2) > + __builtin_abort(); > + return 0; > +} > Index: gcc/testsuite/gcc.dg/torture/pr71598-2.c > =================================================================== > --- gcc/testsuite/gcc.dg/torture/pr71598-2.c (nonexistent) > +++ gcc/testsuite/gcc.dg/torture/pr71598-2.c (working copy) > @@ -0,0 +1,47 @@ > +/* { dg-do run } */ > +/* { dg-additional-options "-fshort-enums" } */ > + > +enum e1 { c1 = -__INT_MAX__ }; > + > +__attribute__((noinline,noclone)) > +int f(enum e1 *p, signed int *q) > +{ > + *p = c1; > + *q = 2; > + return *p; > +} > + > +enum e2 { c2 = __SHRT_MAX__ + 1}; > + > +__attribute__((noinline,noclone)) > +int g(enum e2 *p, unsigned short *q) > +{ > + *p = c2; > + *q = 2; > + return *p; > +} > + > +enum e3 { c3 = __SCHAR_MAX__ }; > + > +__attribute__((noinline,noclone)) > +int h(enum e3 *p, unsigned char *q) > +{ > + *p = c3; > + *q = 2; > + return *p; > +} > + > +int main() > +{ > + signed x; > + unsigned short y; > + unsigned char z; > + > + if (f(&x, &x) != 2) > + __builtin_abort(); > + if (g(&y, &y) != 2) > + __builtin_abort(); > + if (h(&z, &z) != 2) > + __builtin_abort(); > + return 0; > +} >
On 3/26/19 4:40 AM, Richard Biener wrote: > On Mon, 18 Mar 2019, Richard Biener wrote: > >> On Fri, 15 Mar 2019, Jason Merrill wrote: >> >>> On 3/15/19 9:33 AM, Richard Biener wrote: >>>> >>>> The following is an attempt to fix PR71598 where C (and C++?) have >>>> an implementation-defined compatible integer type for each enum >>>> and the TBAA rules mandate that accesses using a compatible type >>>> are allowed. >>> >>> This does not apply to C++; an enum does not alias its underlying type. >> >> Thus the following different patch, introducing c_get_alias_set and >> only doing the special handling for C family frontends (I assume >> that at least ObjC is also affected). >> >> Bootstrap & regtest running on x86_64-unknown-linux-gnu, OK? > > Ping. Also consider the additional testcase below to be added. > > Richard. > > 2019-03-18 Richard Biener <rguenther@suse.de> > > PR c/71598 > * gimple.c: Include langhooks.h. > (gimple_get_alias_set): Treat enumeral types as the underlying > integer type. Won't this affect all languages? Jason
On Thu, 28 Mar 2019, Jason Merrill wrote: > On 3/26/19 4:40 AM, Richard Biener wrote: > > On Mon, 18 Mar 2019, Richard Biener wrote: > > > > > On Fri, 15 Mar 2019, Jason Merrill wrote: > > > > > > > On 3/15/19 9:33 AM, Richard Biener wrote: > > > > > > > > > > The following is an attempt to fix PR71598 where C (and C++?) have > > > > > an implementation-defined compatible integer type for each enum > > > > > and the TBAA rules mandate that accesses using a compatible type > > > > > are allowed. > > > > > > > > This does not apply to C++; an enum does not alias its underlying type. > > > > > > Thus the following different patch, introducing c_get_alias_set and > > > only doing the special handling for C family frontends (I assume > > > that at least ObjC is also affected). > > > > > > Bootstrap & regtest running on x86_64-unknown-linux-gnu, OK? > > > > Ping. Also consider the additional testcase below to be added. > > > > Richard. > > > > 2019-03-18 Richard Biener <rguenther@suse.de> > > > > PR c/71598 > > * gimple.c: Include langhooks.h. > > (gimple_get_alias_set): Treat enumeral types as the underlying > > integer type. > > Won't this affect all languages? It affects all languages during the LTO optimization phase, yes. There's unfortunately no way around that at the moment. Richard.
On Fri, Mar 29, 2019 at 4:48 AM Richard Biener <rguenther@suse.de> wrote: > > On Thu, 28 Mar 2019, Jason Merrill wrote: > > > On 3/26/19 4:40 AM, Richard Biener wrote: > > > On Mon, 18 Mar 2019, Richard Biener wrote: > > > > > > > On Fri, 15 Mar 2019, Jason Merrill wrote: > > > > > > > > > On 3/15/19 9:33 AM, Richard Biener wrote: > > > > > > > > > > > > The following is an attempt to fix PR71598 where C (and C++?) have > > > > > > an implementation-defined compatible integer type for each enum > > > > > > and the TBAA rules mandate that accesses using a compatible type > > > > > > are allowed. > > > > > > > > > > This does not apply to C++; an enum does not alias its underlying type. > > > > > > > > Thus the following different patch, introducing c_get_alias_set and > > > > only doing the special handling for C family frontends (I assume > > > > that at least ObjC is also affected). > > > > > > > > Bootstrap & regtest running on x86_64-unknown-linux-gnu, OK? > > > > > > Ping. Also consider the additional testcase below to be added. > > > > > > Richard. > > > > > > 2019-03-18 Richard Biener <rguenther@suse.de> > > > > > > PR c/71598 > > > * gimple.c: Include langhooks.h. > > > (gimple_get_alias_set): Treat enumeral types as the underlying > > > integer type. > > > > Won't this affect all languages? > > It affects all languages during the LTO optimization phase, yes. > There's unfortunately no way around that at the moment. Ah, well. Looks good to me, then. Jason > Richard.
On 3/29/19 9:09 AM, Jason Merrill wrote: > On Fri, Mar 29, 2019 at 4:48 AM Richard Biener <rguenther@suse.de> wrote: >> >> On Thu, 28 Mar 2019, Jason Merrill wrote: >> >>> On 3/26/19 4:40 AM, Richard Biener wrote: >>>> On Mon, 18 Mar 2019, Richard Biener wrote: >>>> >>>>> On Fri, 15 Mar 2019, Jason Merrill wrote: >>>>> >>>>>> On 3/15/19 9:33 AM, Richard Biener wrote: >>>>>>> >>>>>>> The following is an attempt to fix PR71598 where C (and C++?) have >>>>>>> an implementation-defined compatible integer type for each enum >>>>>>> and the TBAA rules mandate that accesses using a compatible type >>>>>>> are allowed. >>>>>> >>>>>> This does not apply to C++; an enum does not alias its underlying type. >>>>> >>>>> Thus the following different patch, introducing c_get_alias_set and >>>>> only doing the special handling for C family frontends (I assume >>>>> that at least ObjC is also affected). >>>>> >>>>> Bootstrap & regtest running on x86_64-unknown-linux-gnu, OK? >>>> >>>> Ping. Also consider the additional testcase below to be added. >>>> >>>> Richard. >>>> >>>> 2019-03-18 Richard Biener <rguenther@suse.de> >>>> >>>> PR c/71598 >>>> * gimple.c: Include langhooks.h. >>>> (gimple_get_alias_set): Treat enumeral types as the underlying >>>> integer type. >>> >>> Won't this affect all languages? >> >> It affects all languages during the LTO optimization phase, yes. >> There's unfortunately no way around that at the moment. > > Ah, well. Looks good to me, then. Likewise. And with Joseph largely offline right now, that's going to have to be sufficient :-) jeff
Hi! On Fri, 29 Mar 2019 at 20:02, Jeff Law <law@redhat.com> wrote: > > On 3/29/19 9:09 AM, Jason Merrill wrote: > > On Fri, Mar 29, 2019 at 4:48 AM Richard Biener <rguenther@suse.de> wrote: > >> > >> On Thu, 28 Mar 2019, Jason Merrill wrote: > >> > >>> On 3/26/19 4:40 AM, Richard Biener wrote: > >>>> On Mon, 18 Mar 2019, Richard Biener wrote: > >>>> > >>>>> On Fri, 15 Mar 2019, Jason Merrill wrote: > >>>>> > >>>>>> On 3/15/19 9:33 AM, Richard Biener wrote: > >>>>>>> > >>>>>>> The following is an attempt to fix PR71598 where C (and C++?) have > >>>>>>> an implementation-defined compatible integer type for each enum > >>>>>>> and the TBAA rules mandate that accesses using a compatible type > >>>>>>> are allowed. > >>>>>> > >>>>>> This does not apply to C++; an enum does not alias its underlying type. > >>>>> > >>>>> Thus the following different patch, introducing c_get_alias_set and > >>>>> only doing the special handling for C family frontends (I assume > >>>>> that at least ObjC is also affected). > >>>>> > >>>>> Bootstrap & regtest running on x86_64-unknown-linux-gnu, OK? > >>>> > >>>> Ping. Also consider the additional testcase below to be added. > >>>> > >>>> Richard. > >>>> > >>>> 2019-03-18 Richard Biener <rguenther@suse.de> > >>>> > >>>> PR c/71598 > >>>> * gimple.c: Include langhooks.h. > >>>> (gimple_get_alias_set): Treat enumeral types as the underlying > >>>> integer type. > >>> > >>> Won't this affect all languages? > >> > >> It affects all languages during the LTO optimization phase, yes. > >> There's unfortunately no way around that at the moment. > > > > Ah, well. Looks good to me, then. > Likewise. And with Joseph largely offline right now, that's going to > have to be sufficient :-) > I've noticed minor new errors at link time on arm with the 2 new testcases. pr71598-1.c complains on arm-none-eabi because arm-none-eabi/bin/ld: warning: /ccu5w26t.o uses 32-bit enums yet the output is to use variable-size enums; use of enum values across objects may fail conversely, pr71598-2.c complains on arm-none-linux-gnueabi* because: arm-none-linux-gnueabi/bin/ld: warning: /ccl5OUKi.o uses variable-size enums yet the output is to use 32-bit enums; use of enum values across objects may fail In both cases this is because crt0.o is built with the opposite (default) short-enum option than the testcase, so the linker sees a mismatch between crt0.o and pr71598-X.o. Shall I add target-dependent dg-warning directives in the testcases, or is there a better way? Thanks, Christophe > jeff
On Wed, 3 Apr 2019, Christophe Lyon wrote: > Hi! > > On Fri, 29 Mar 2019 at 20:02, Jeff Law <law@redhat.com> wrote: > > > > On 3/29/19 9:09 AM, Jason Merrill wrote: > > > On Fri, Mar 29, 2019 at 4:48 AM Richard Biener <rguenther@suse.de> wrote: > > >> > > >> On Thu, 28 Mar 2019, Jason Merrill wrote: > > >> > > >>> On 3/26/19 4:40 AM, Richard Biener wrote: > > >>>> On Mon, 18 Mar 2019, Richard Biener wrote: > > >>>> > > >>>>> On Fri, 15 Mar 2019, Jason Merrill wrote: > > >>>>> > > >>>>>> On 3/15/19 9:33 AM, Richard Biener wrote: > > >>>>>>> > > >>>>>>> The following is an attempt to fix PR71598 where C (and C++?) have > > >>>>>>> an implementation-defined compatible integer type for each enum > > >>>>>>> and the TBAA rules mandate that accesses using a compatible type > > >>>>>>> are allowed. > > >>>>>> > > >>>>>> This does not apply to C++; an enum does not alias its underlying type. > > >>>>> > > >>>>> Thus the following different patch, introducing c_get_alias_set and > > >>>>> only doing the special handling for C family frontends (I assume > > >>>>> that at least ObjC is also affected). > > >>>>> > > >>>>> Bootstrap & regtest running on x86_64-unknown-linux-gnu, OK? > > >>>> > > >>>> Ping. Also consider the additional testcase below to be added. > > >>>> > > >>>> Richard. > > >>>> > > >>>> 2019-03-18 Richard Biener <rguenther@suse.de> > > >>>> > > >>>> PR c/71598 > > >>>> * gimple.c: Include langhooks.h. > > >>>> (gimple_get_alias_set): Treat enumeral types as the underlying > > >>>> integer type. > > >>> > > >>> Won't this affect all languages? > > >> > > >> It affects all languages during the LTO optimization phase, yes. > > >> There's unfortunately no way around that at the moment. > > > > > > Ah, well. Looks good to me, then. > > Likewise. And with Joseph largely offline right now, that's going to > > have to be sufficient :-) > > > > > I've noticed minor new errors at link time on arm with the 2 new testcases. > pr71598-1.c complains on arm-none-eabi because > arm-none-eabi/bin/ld: warning: /ccu5w26t.o uses 32-bit enums yet the > output is to use variable-size enums; use of enum values across > objects may fail > > conversely, pr71598-2.c complains on arm-none-linux-gnueabi* because: > arm-none-linux-gnueabi/bin/ld: warning: /ccl5OUKi.o uses variable-size > enums yet the output is to use 32-bit enums; use of enum values across > objects may fail > > In both cases this is because crt0.o is built with the opposite > (default) short-enum option than the testcase, so the linker sees a > mismatch between crt0.o and pr71598-X.o. > > Shall I add target-dependent dg-warning directives in the testcases, > or is there a better way? Maybe dg-skip the test for target_short_enums which seems to exist? Can you try if that works and if so, commit the fix? Thanks, Richard.
On Wed, 3 Apr 2019 at 10:24, Richard Biener <rguenther@suse.de> wrote: > > On Wed, 3 Apr 2019, Christophe Lyon wrote: > > > Hi! > > > > On Fri, 29 Mar 2019 at 20:02, Jeff Law <law@redhat.com> wrote: > > > > > > On 3/29/19 9:09 AM, Jason Merrill wrote: > > > > On Fri, Mar 29, 2019 at 4:48 AM Richard Biener <rguenther@suse.de> wrote: > > > >> > > > >> On Thu, 28 Mar 2019, Jason Merrill wrote: > > > >> > > > >>> On 3/26/19 4:40 AM, Richard Biener wrote: > > > >>>> On Mon, 18 Mar 2019, Richard Biener wrote: > > > >>>> > > > >>>>> On Fri, 15 Mar 2019, Jason Merrill wrote: > > > >>>>> > > > >>>>>> On 3/15/19 9:33 AM, Richard Biener wrote: > > > >>>>>>> > > > >>>>>>> The following is an attempt to fix PR71598 where C (and C++?) have > > > >>>>>>> an implementation-defined compatible integer type for each enum > > > >>>>>>> and the TBAA rules mandate that accesses using a compatible type > > > >>>>>>> are allowed. > > > >>>>>> > > > >>>>>> This does not apply to C++; an enum does not alias its underlying type. > > > >>>>> > > > >>>>> Thus the following different patch, introducing c_get_alias_set and > > > >>>>> only doing the special handling for C family frontends (I assume > > > >>>>> that at least ObjC is also affected). > > > >>>>> > > > >>>>> Bootstrap & regtest running on x86_64-unknown-linux-gnu, OK? > > > >>>> > > > >>>> Ping. Also consider the additional testcase below to be added. > > > >>>> > > > >>>> Richard. > > > >>>> > > > >>>> 2019-03-18 Richard Biener <rguenther@suse.de> > > > >>>> > > > >>>> PR c/71598 > > > >>>> * gimple.c: Include langhooks.h. > > > >>>> (gimple_get_alias_set): Treat enumeral types as the underlying > > > >>>> integer type. > > > >>> > > > >>> Won't this affect all languages? > > > >> > > > >> It affects all languages during the LTO optimization phase, yes. > > > >> There's unfortunately no way around that at the moment. > > > > > > > > Ah, well. Looks good to me, then. > > > Likewise. And with Joseph largely offline right now, that's going to > > > have to be sufficient :-) > > > > > > > > > I've noticed minor new errors at link time on arm with the 2 new testcases. > > pr71598-1.c complains on arm-none-eabi because > > arm-none-eabi/bin/ld: warning: /ccu5w26t.o uses 32-bit enums yet the > > output is to use variable-size enums; use of enum values across > > objects may fail > > > > conversely, pr71598-2.c complains on arm-none-linux-gnueabi* because: > > arm-none-linux-gnueabi/bin/ld: warning: /ccl5OUKi.o uses variable-size > > enums yet the output is to use 32-bit enums; use of enum values across > > objects may fail > > > > In both cases this is because crt0.o is built with the opposite > > (default) short-enum option than the testcase, so the linker sees a > > mismatch between crt0.o and pr71598-X.o. > > > > Shall I add target-dependent dg-warning directives in the testcases, > > or is there a better way? > > Maybe dg-skip the test for target_short_enums which seems to exist? > > Can you try if that works and if so, commit the fix? > Thanks, here is what I have committed as r270126. (skip one test on short_enums targets, skip the other one on non-short_enums targets) Christophe > Thanks, > Richard. 2019-04-13 Christophe Lyon <christophe.lyon@linaro.org> PR c/71598 * gcc.dg/torture/pr71598-1.c: Skip if short_enums target. * gcc.dg/torture/pr71598-2.c: Skip if not short_enums target. Index: gcc/testsuite/gcc.dg/torture/pr71598-1.c =================================================================== --- gcc/testsuite/gcc.dg/torture/pr71598-1.c (revision 270125) +++ gcc/testsuite/gcc.dg/torture/pr71598-1.c (revision 270126) @@ -1,4 +1,5 @@ /* { dg-do run } */ +/* { dg-skip-if "" { short_enums } } */ /* { dg-additional-options "-fno-short-enums" } */ enum e1 { c1 }; Index: gcc/testsuite/gcc.dg/torture/pr71598-2.c =================================================================== --- gcc/testsuite/gcc.dg/torture/pr71598-2.c (revision 270125) +++ gcc/testsuite/gcc.dg/torture/pr71598-2.c (revision 270126) @@ -1,4 +1,5 @@ /* { dg-do run } */ +/* { dg-skip-if "" { ! short_enums } } */ /* { dg-additional-options "-fshort-enums" } */ enum e1 { c1 = -__INT_MAX__ };
On Wed, 3 Apr 2019 at 15:19, Christophe Lyon <christophe.lyon@linaro.org> wrote: > > On Wed, 3 Apr 2019 at 10:24, Richard Biener <rguenther@suse.de> wrote: > > > > On Wed, 3 Apr 2019, Christophe Lyon wrote: > > > > > Hi! > > > > > > On Fri, 29 Mar 2019 at 20:02, Jeff Law <law@redhat.com> wrote: > > > > > > > > On 3/29/19 9:09 AM, Jason Merrill wrote: > > > > > On Fri, Mar 29, 2019 at 4:48 AM Richard Biener <rguenther@suse.de> wrote: > > > > >> > > > > >> On Thu, 28 Mar 2019, Jason Merrill wrote: > > > > >> > > > > >>> On 3/26/19 4:40 AM, Richard Biener wrote: > > > > >>>> On Mon, 18 Mar 2019, Richard Biener wrote: > > > > >>>> > > > > >>>>> On Fri, 15 Mar 2019, Jason Merrill wrote: > > > > >>>>> > > > > >>>>>> On 3/15/19 9:33 AM, Richard Biener wrote: > > > > >>>>>>> > > > > >>>>>>> The following is an attempt to fix PR71598 where C (and C++?) have > > > > >>>>>>> an implementation-defined compatible integer type for each enum > > > > >>>>>>> and the TBAA rules mandate that accesses using a compatible type > > > > >>>>>>> are allowed. > > > > >>>>>> > > > > >>>>>> This does not apply to C++; an enum does not alias its underlying type. > > > > >>>>> > > > > >>>>> Thus the following different patch, introducing c_get_alias_set and > > > > >>>>> only doing the special handling for C family frontends (I assume > > > > >>>>> that at least ObjC is also affected). > > > > >>>>> > > > > >>>>> Bootstrap & regtest running on x86_64-unknown-linux-gnu, OK? > > > > >>>> > > > > >>>> Ping. Also consider the additional testcase below to be added. > > > > >>>> > > > > >>>> Richard. > > > > >>>> > > > > >>>> 2019-03-18 Richard Biener <rguenther@suse.de> > > > > >>>> > > > > >>>> PR c/71598 > > > > >>>> * gimple.c: Include langhooks.h. > > > > >>>> (gimple_get_alias_set): Treat enumeral types as the underlying > > > > >>>> integer type. > > > > >>> > > > > >>> Won't this affect all languages? > > > > >> > > > > >> It affects all languages during the LTO optimization phase, yes. > > > > >> There's unfortunately no way around that at the moment. > > > > > > > > > > Ah, well. Looks good to me, then. > > > > Likewise. And with Joseph largely offline right now, that's going to > > > > have to be sufficient :-) > > > > > > > > > > > > > I've noticed minor new errors at link time on arm with the 2 new testcases. > > > pr71598-1.c complains on arm-none-eabi because > > > arm-none-eabi/bin/ld: warning: /ccu5w26t.o uses 32-bit enums yet the > > > output is to use variable-size enums; use of enum values across > > > objects may fail > > > > > > conversely, pr71598-2.c complains on arm-none-linux-gnueabi* because: > > > arm-none-linux-gnueabi/bin/ld: warning: /ccl5OUKi.o uses variable-size > > > enums yet the output is to use 32-bit enums; use of enum values across > > > objects may fail > > > > > > In both cases this is because crt0.o is built with the opposite > > > (default) short-enum option than the testcase, so the linker sees a > > > mismatch between crt0.o and pr71598-X.o. > > > > > > Shall I add target-dependent dg-warning directives in the testcases, > > > or is there a better way? > > > > Maybe dg-skip the test for target_short_enums which seems to exist? > > > > Can you try if that works and if so, commit the fix? > > > > Thanks, here is what I have committed as r270126. > (skip one test on short_enums targets, skip the other one on > non-short_enums targets) > However this has the drawback that pr71598-2.c is now skipped on aarch64 (and probably many other targets). > Christophe > > > Thanks, > > Richard.
On Wed, Apr 3, 2019 at 6:19 AM Christophe Lyon <christophe.lyon@linaro.org> wrote: > > Thanks, here is what I have committed as r270126. > (skip one test on short_enums targets, skip the other one on > non-short_enums targets) I noticed that your testsuite/ChangeLog entry is marked 2019-04-13, but it should perhaps be 2019-04-03. Ian
On Wed, 3 Apr 2019 at 20:24, Ian Lance Taylor <iant@google.com> wrote: > > On Wed, Apr 3, 2019 at 6:19 AM Christophe Lyon > <christophe.lyon@linaro.org> wrote: > > > > Thanks, here is what I have committed as r270126. > > (skip one test on short_enums targets, skip the other one on > > non-short_enums targets) > > I noticed that your testsuite/ChangeLog entry is marked 2019-04-13, > but it should perhaps be 2019-04-03. > Oops, sorry. I've just fixed it, thanks Christophe > Ian
On Wed, 3 Apr 2019, Christophe Lyon wrote: > On Wed, 3 Apr 2019 at 15:19, Christophe Lyon <christophe.lyon@linaro.org> wrote: > > > > On Wed, 3 Apr 2019 at 10:24, Richard Biener <rguenther@suse.de> wrote: > > > > > > On Wed, 3 Apr 2019, Christophe Lyon wrote: > > > > > > > Hi! > > > > > > > > On Fri, 29 Mar 2019 at 20:02, Jeff Law <law@redhat.com> wrote: > > > > > > > > > > On 3/29/19 9:09 AM, Jason Merrill wrote: > > > > > > On Fri, Mar 29, 2019 at 4:48 AM Richard Biener <rguenther@suse.de> wrote: > > > > > >> > > > > > >> On Thu, 28 Mar 2019, Jason Merrill wrote: > > > > > >> > > > > > >>> On 3/26/19 4:40 AM, Richard Biener wrote: > > > > > >>>> On Mon, 18 Mar 2019, Richard Biener wrote: > > > > > >>>> > > > > > >>>>> On Fri, 15 Mar 2019, Jason Merrill wrote: > > > > > >>>>> > > > > > >>>>>> On 3/15/19 9:33 AM, Richard Biener wrote: > > > > > >>>>>>> > > > > > >>>>>>> The following is an attempt to fix PR71598 where C (and C++?) have > > > > > >>>>>>> an implementation-defined compatible integer type for each enum > > > > > >>>>>>> and the TBAA rules mandate that accesses using a compatible type > > > > > >>>>>>> are allowed. > > > > > >>>>>> > > > > > >>>>>> This does not apply to C++; an enum does not alias its underlying type. > > > > > >>>>> > > > > > >>>>> Thus the following different patch, introducing c_get_alias_set and > > > > > >>>>> only doing the special handling for C family frontends (I assume > > > > > >>>>> that at least ObjC is also affected). > > > > > >>>>> > > > > > >>>>> Bootstrap & regtest running on x86_64-unknown-linux-gnu, OK? > > > > > >>>> > > > > > >>>> Ping. Also consider the additional testcase below to be added. > > > > > >>>> > > > > > >>>> Richard. > > > > > >>>> > > > > > >>>> 2019-03-18 Richard Biener <rguenther@suse.de> > > > > > >>>> > > > > > >>>> PR c/71598 > > > > > >>>> * gimple.c: Include langhooks.h. > > > > > >>>> (gimple_get_alias_set): Treat enumeral types as the underlying > > > > > >>>> integer type. > > > > > >>> > > > > > >>> Won't this affect all languages? > > > > > >> > > > > > >> It affects all languages during the LTO optimization phase, yes. > > > > > >> There's unfortunately no way around that at the moment. > > > > > > > > > > > > Ah, well. Looks good to me, then. > > > > > Likewise. And with Joseph largely offline right now, that's going to > > > > > have to be sufficient :-) > > > > > > > > > > > > > > > > > I've noticed minor new errors at link time on arm with the 2 new testcases. > > > > pr71598-1.c complains on arm-none-eabi because > > > > arm-none-eabi/bin/ld: warning: /ccu5w26t.o uses 32-bit enums yet the > > > > output is to use variable-size enums; use of enum values across > > > > objects may fail > > > > > > > > conversely, pr71598-2.c complains on arm-none-linux-gnueabi* because: > > > > arm-none-linux-gnueabi/bin/ld: warning: /ccl5OUKi.o uses variable-size > > > > enums yet the output is to use 32-bit enums; use of enum values across > > > > objects may fail > > > > > > > > In both cases this is because crt0.o is built with the opposite > > > > (default) short-enum option than the testcase, so the linker sees a > > > > mismatch between crt0.o and pr71598-X.o. > > > > > > > > Shall I add target-dependent dg-warning directives in the testcases, > > > > or is there a better way? > > > > > > Maybe dg-skip the test for target_short_enums which seems to exist? > > > > > > Can you try if that works and if so, commit the fix? > > > > > > > Thanks, here is what I have committed as r270126. > > (skip one test on short_enums targets, skip the other one on > > non-short_enums targets) > > > However this has the drawback that pr71598-2.c is now skipped on > aarch64 (and probably many other targets). Hmm, yeah. The question is why do some targets warn and some do not? Anyway, can you investigate a dg-prune/message solution instead? Thanks, Richard. > > Christophe > > > > > Thanks, > > > Richard. >
On Fri, 5 Apr 2019 at 09:38, Richard Biener <rguenther@suse.de> wrote: > > On Wed, 3 Apr 2019, Christophe Lyon wrote: > > > On Wed, 3 Apr 2019 at 15:19, Christophe Lyon <christophe.lyon@linaro.org> wrote: > > > > > > On Wed, 3 Apr 2019 at 10:24, Richard Biener <rguenther@suse.de> wrote: > > > > > > > > On Wed, 3 Apr 2019, Christophe Lyon wrote: > > > > > > > > > Hi! > > > > > > > > > > On Fri, 29 Mar 2019 at 20:02, Jeff Law <law@redhat.com> wrote: > > > > > > > > > > > > On 3/29/19 9:09 AM, Jason Merrill wrote: > > > > > > > On Fri, Mar 29, 2019 at 4:48 AM Richard Biener <rguenther@suse.de> wrote: > > > > > > >> > > > > > > >> On Thu, 28 Mar 2019, Jason Merrill wrote: > > > > > > >> > > > > > > >>> On 3/26/19 4:40 AM, Richard Biener wrote: > > > > > > >>>> On Mon, 18 Mar 2019, Richard Biener wrote: > > > > > > >>>> > > > > > > >>>>> On Fri, 15 Mar 2019, Jason Merrill wrote: > > > > > > >>>>> > > > > > > >>>>>> On 3/15/19 9:33 AM, Richard Biener wrote: > > > > > > >>>>>>> > > > > > > >>>>>>> The following is an attempt to fix PR71598 where C (and C++?) have > > > > > > >>>>>>> an implementation-defined compatible integer type for each enum > > > > > > >>>>>>> and the TBAA rules mandate that accesses using a compatible type > > > > > > >>>>>>> are allowed. > > > > > > >>>>>> > > > > > > >>>>>> This does not apply to C++; an enum does not alias its underlying type. > > > > > > >>>>> > > > > > > >>>>> Thus the following different patch, introducing c_get_alias_set and > > > > > > >>>>> only doing the special handling for C family frontends (I assume > > > > > > >>>>> that at least ObjC is also affected). > > > > > > >>>>> > > > > > > >>>>> Bootstrap & regtest running on x86_64-unknown-linux-gnu, OK? > > > > > > >>>> > > > > > > >>>> Ping. Also consider the additional testcase below to be added. > > > > > > >>>> > > > > > > >>>> Richard. > > > > > > >>>> > > > > > > >>>> 2019-03-18 Richard Biener <rguenther@suse.de> > > > > > > >>>> > > > > > > >>>> PR c/71598 > > > > > > >>>> * gimple.c: Include langhooks.h. > > > > > > >>>> (gimple_get_alias_set): Treat enumeral types as the underlying > > > > > > >>>> integer type. > > > > > > >>> > > > > > > >>> Won't this affect all languages? > > > > > > >> > > > > > > >> It affects all languages during the LTO optimization phase, yes. > > > > > > >> There's unfortunately no way around that at the moment. > > > > > > > > > > > > > > Ah, well. Looks good to me, then. > > > > > > Likewise. And with Joseph largely offline right now, that's going to > > > > > > have to be sufficient :-) > > > > > > > > > > > > > > > > > > > > > I've noticed minor new errors at link time on arm with the 2 new testcases. > > > > > pr71598-1.c complains on arm-none-eabi because > > > > > arm-none-eabi/bin/ld: warning: /ccu5w26t.o uses 32-bit enums yet the > > > > > output is to use variable-size enums; use of enum values across > > > > > objects may fail > > > > > > > > > > conversely, pr71598-2.c complains on arm-none-linux-gnueabi* because: > > > > > arm-none-linux-gnueabi/bin/ld: warning: /ccl5OUKi.o uses variable-size > > > > > enums yet the output is to use 32-bit enums; use of enum values across > > > > > objects may fail > > > > > > > > > > In both cases this is because crt0.o is built with the opposite > > > > > (default) short-enum option than the testcase, so the linker sees a > > > > > mismatch between crt0.o and pr71598-X.o. > > > > > > > > > > Shall I add target-dependent dg-warning directives in the testcases, > > > > > or is there a better way? > > > > > > > > Maybe dg-skip the test for target_short_enums which seems to exist? > > > > > > > > Can you try if that works and if so, commit the fix? > > > > > > > > > > Thanks, here is what I have committed as r270126. > > > (skip one test on short_enums targets, skip the other one on > > > non-short_enums targets) > > > > > However this has the drawback that pr71598-2.c is now skipped on > > aarch64 (and probably many other targets). > > Hmm, yeah. The question is why do some targets warn and some do not? Well, that's a target-dependent warning from the linker, because on arm there is an attribute to describe the type of enums. But indeed it seems it should apply to all targets, however it could trigger lots of false positives. > Anyway, can you investigate a dg-prune/message solution instead? Sure, it seems to work with: /* { dg-prune-output "use of enum values across objects may fail" } */ which is not target-dependent though. I'll commit that if there is no objection. Christophe > Thanks, > Richard. > > > > Christophe > > > > > > > Thanks, > > > > Richard. > > > > -- > Richard Biener <rguenther@suse.de> > SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; > GF: Felix Imendörffer, Mary Higgins, Sri Rasiah; HRB 21284 (AG Nürnberg)
On Fri, 5 Apr 2019, Christophe Lyon wrote: > On Fri, 5 Apr 2019 at 09:38, Richard Biener <rguenther@suse.de> wrote: > > > > On Wed, 3 Apr 2019, Christophe Lyon wrote: > > > > > On Wed, 3 Apr 2019 at 15:19, Christophe Lyon <christophe.lyon@linaro.org> wrote: > > > > > > > > On Wed, 3 Apr 2019 at 10:24, Richard Biener <rguenther@suse.de> wrote: > > > > > > > > > > On Wed, 3 Apr 2019, Christophe Lyon wrote: > > > > > > > > > > > Hi! > > > > > > > > > > > > On Fri, 29 Mar 2019 at 20:02, Jeff Law <law@redhat.com> wrote: > > > > > > > > > > > > > > On 3/29/19 9:09 AM, Jason Merrill wrote: > > > > > > > > On Fri, Mar 29, 2019 at 4:48 AM Richard Biener <rguenther@suse.de> wrote: > > > > > > > >> > > > > > > > >> On Thu, 28 Mar 2019, Jason Merrill wrote: > > > > > > > >> > > > > > > > >>> On 3/26/19 4:40 AM, Richard Biener wrote: > > > > > > > >>>> On Mon, 18 Mar 2019, Richard Biener wrote: > > > > > > > >>>> > > > > > > > >>>>> On Fri, 15 Mar 2019, Jason Merrill wrote: > > > > > > > >>>>> > > > > > > > >>>>>> On 3/15/19 9:33 AM, Richard Biener wrote: > > > > > > > >>>>>>> > > > > > > > >>>>>>> The following is an attempt to fix PR71598 where C (and C++?) have > > > > > > > >>>>>>> an implementation-defined compatible integer type for each enum > > > > > > > >>>>>>> and the TBAA rules mandate that accesses using a compatible type > > > > > > > >>>>>>> are allowed. > > > > > > > >>>>>> > > > > > > > >>>>>> This does not apply to C++; an enum does not alias its underlying type. > > > > > > > >>>>> > > > > > > > >>>>> Thus the following different patch, introducing c_get_alias_set and > > > > > > > >>>>> only doing the special handling for C family frontends (I assume > > > > > > > >>>>> that at least ObjC is also affected). > > > > > > > >>>>> > > > > > > > >>>>> Bootstrap & regtest running on x86_64-unknown-linux-gnu, OK? > > > > > > > >>>> > > > > > > > >>>> Ping. Also consider the additional testcase below to be added. > > > > > > > >>>> > > > > > > > >>>> Richard. > > > > > > > >>>> > > > > > > > >>>> 2019-03-18 Richard Biener <rguenther@suse.de> > > > > > > > >>>> > > > > > > > >>>> PR c/71598 > > > > > > > >>>> * gimple.c: Include langhooks.h. > > > > > > > >>>> (gimple_get_alias_set): Treat enumeral types as the underlying > > > > > > > >>>> integer type. > > > > > > > >>> > > > > > > > >>> Won't this affect all languages? > > > > > > > >> > > > > > > > >> It affects all languages during the LTO optimization phase, yes. > > > > > > > >> There's unfortunately no way around that at the moment. > > > > > > > > > > > > > > > > Ah, well. Looks good to me, then. > > > > > > > Likewise. And with Joseph largely offline right now, that's going to > > > > > > > have to be sufficient :-) > > > > > > > > > > > > > > > > > > > > > > > > > I've noticed minor new errors at link time on arm with the 2 new testcases. > > > > > > pr71598-1.c complains on arm-none-eabi because > > > > > > arm-none-eabi/bin/ld: warning: /ccu5w26t.o uses 32-bit enums yet the > > > > > > output is to use variable-size enums; use of enum values across > > > > > > objects may fail > > > > > > > > > > > > conversely, pr71598-2.c complains on arm-none-linux-gnueabi* because: > > > > > > arm-none-linux-gnueabi/bin/ld: warning: /ccl5OUKi.o uses variable-size > > > > > > enums yet the output is to use 32-bit enums; use of enum values across > > > > > > objects may fail > > > > > > > > > > > > In both cases this is because crt0.o is built with the opposite > > > > > > (default) short-enum option than the testcase, so the linker sees a > > > > > > mismatch between crt0.o and pr71598-X.o. > > > > > > > > > > > > Shall I add target-dependent dg-warning directives in the testcases, > > > > > > or is there a better way? > > > > > > > > > > Maybe dg-skip the test for target_short_enums which seems to exist? > > > > > > > > > > Can you try if that works and if so, commit the fix? > > > > > > > > > > > > > Thanks, here is what I have committed as r270126. > > > > (skip one test on short_enums targets, skip the other one on > > > > non-short_enums targets) > > > > > > > However this has the drawback that pr71598-2.c is now skipped on > > > aarch64 (and probably many other targets). > > > > Hmm, yeah. The question is why do some targets warn and some do not? > Well, that's a target-dependent warning from the linker, because on arm > there is an attribute to describe the type of enums. But indeed it seems it > should apply to all targets, however it could trigger lots of false positives. > > > Anyway, can you investigate a dg-prune/message solution instead? > Sure, it seems to work with: > /* { dg-prune-output "use of enum values across objects may fail" } */ > which is not target-dependent though. > > I'll commit that if there is no objection. Works for me. Richard. > Christophe > > > > > Thanks, > > Richard. > > > > > > Christophe > > > > > > > > > Thanks, > > > > > Richard. > > > > > > > -- > > Richard Biener <rguenther@suse.de> > > SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; > > GF: Felix Imendörffer, Mary Higgins, Sri Rasiah; HRB 21284 (AG Nürnberg) >
Index: gcc/gimple.c =================================================================== --- gcc/gimple.c (revision 269704) +++ gcc/gimple.c (working copy) @@ -44,6 +44,7 @@ along with GCC; see the file COPYING3. #include "stringpool.h" #include "attribs.h" #include "asan.h" +#include "langhooks.h" /* All the tuples have their operand vector (if present) at the very bottom @@ -2587,6 +2588,16 @@ gimple_get_alias_set (tree t) return get_alias_set (t1); } + /* Allow aliasing between enumeral types and the underlying + integer type. This is required for C since those are + compatible types. */ + else if (TREE_CODE (t) == ENUMERAL_TYPE) + { + tree t1 = lang_hooks.types.type_for_size (tree_to_uhwi (TYPE_SIZE (t)), + false /* short-cut above */); + return get_alias_set (t1); + } + return -1; } Index: gcc/c-family/c-common.c =================================================================== --- gcc/c-family/c-common.c (revision 269704) +++ gcc/c-family/c-common.c (working copy) @@ -3681,6 +3681,15 @@ c_common_get_alias_set (tree t) return get_alias_set (t1); } + /* Allow aliasing between enumeral types and the underlying + integer type. This is required since those are compatible types. */ + else if (TREE_CODE (t) == ENUMERAL_TYPE) + { + tree t1 = c_common_type_for_size (tree_to_uhwi (TYPE_SIZE (t)), + false /* short-cut above */); + return get_alias_set (t1); + } + return -1; } Index: gcc/testsuite/c-c++-common/torture/pr71598-1.c =================================================================== --- gcc/testsuite/c-c++-common/torture/pr71598-1.c (nonexistent) +++ gcc/testsuite/c-c++-common/torture/pr71598-1.c (working copy) @@ -0,0 +1,21 @@ +/* { dg-do run } */ +/* { dg-additional-options "-fno-short-enums" } */ + +enum e1 { c1 }; + +__attribute__((noinline,noclone)) +int f(enum e1 *p, unsigned *q) +{ + *p = c1; + *q = 2; + return *p; +} + +int main() +{ + unsigned x; + + if (f((enum e1 *)&x, &x) != 2) + __builtin_abort(); + return 0; +} Index: gcc/testsuite/c-c++-common/torture/pr71598-2.c =================================================================== --- gcc/testsuite/c-c++-common/torture/pr71598-2.c (nonexistent) +++ gcc/testsuite/c-c++-common/torture/pr71598-2.c (working copy) @@ -0,0 +1,47 @@ +/* { dg-do run } */ +/* { dg-additional-options "-fshort-enums" } */ + +enum e1 { c1 = -__INT_MAX__ }; + +__attribute__((noinline,noclone)) +int f(enum e1 *p, signed int *q) +{ + *p = c1; + *q = 2; + return *p; +} + +enum e2 { c2 = __SHRT_MAX__ + 1}; + +__attribute__((noinline,noclone)) +int g(enum e2 *p, unsigned short *q) +{ + *p = c2; + *q = 2; + return *p; +} + +enum e3 { c3 = __SCHAR_MAX__ }; + +__attribute__((noinline,noclone)) +int h(enum e3 *p, unsigned char *q) +{ + *p = c3; + *q = 2; + return *p; +} + +int main() +{ + signed x; + unsigned short y; + unsigned char z; + + if (f((enum e1 *)&x, &x) != 2) + __builtin_abort(); + if (g((enum e2 *)&y, &y) != 2) + __builtin_abort(); + if (h((enum e3 *)&z, &z) != 2) + __builtin_abort(); + return 0; +}