Re: [PATCH][LTO] Fix PR48437
On Wed, 7 Dec 2011, Diego Novillo wrote: > On Wed, Dec 7, 2011 at 11:16, Richard Guenther wrote: > > > I'm going to apply it tomorrow, when full testing hopefully finished > > Sure. But remember the zombie kitties! I have applied the fix for PR48437, the fix for PR49945 required adjustment as otherwise we'd ICE gcc.dg/lto/20090706-1_0.c in the type checker. We also have to localize FIELD_DECLs of variable size types. Thus I'm re-testing the following and will commit that variant if it succeeds. I suppose at some point we need to look at the efficiency of the variably_modified_type_p call, as tree_is_indexable is called for each component type and variably_modified_type_p recurses itself (thus, overall this is quadratic, but with cheap constant factor as we are calling it with a NULL function arg). Richard. 2011-12-08 Richard Guenther PR lto/49945 * lto-streamer-out.c (tree_is_indexable): Localize variably modified types and their FIELD_DECLs. Index: gcc/lto-streamer-out.c === --- gcc/lto-streamer-out.c (revision 182101) +++ gcc/lto-streamer-out.c (working copy) @@ -139,6 +139,16 @@ tree_is_indexable (tree t) && DECL_CONTEXT (t) && TREE_CODE (DECL_CONTEXT (t)) == FUNCTION_DECL) return false; + /* Variably modified types need to be streamed alongside function + bodies because they can refer to local entities. Together with + them we have to localize their members as well. + ??? In theory that includes non-FIELD_DECLs as well. */ + else if (TYPE_P (t) + && variably_modified_type_p (t, NULL_TREE)) +return false; + else if (TREE_CODE (t) == FIELD_DECL + && variably_modified_type_p (DECL_CONTEXT (t), NULL_TREE)) +return false; else return (TYPE_P (t) || DECL_P (t) || TREE_CODE (t) == SSA_NAME); }
Re: [PATCH][LTO] Fix PR48437
On Wed, Dec 7, 2011 at 11:16, Richard Guenther wrote: > I'm going to apply it tomorrow, when full testing hopefully finished Sure. But remember the zombie kitties! Diego.
Re: [PATCH][LTO] Fix PR48437
On Wed, 7 Dec 2011, Diego Novillo wrote:
> On 12/07/11 10:54, Richard Guenther wrote:
> > On Wed, 7 Dec 2011, Diego Novillo wrote:
> >
> > > On 12/07/11 10:46, Richard Guenther wrote:
> > > > On Wed, 7 Dec 2011, Diego Novillo wrote:
> > > >
> > > > > On 12/07/11 09:52, Richard Guenther wrote:
> > > > >
> > > > > > Index: gcc/lto-streamer-out.c
> > > > > > ===
> > > > > > *** gcc/lto-streamer-out.c (revision 182081)
> > > > > > --- gcc/lto-streamer-out.c (working copy)
> > > > > > *** tree_is_indexable (tree t)
> > > > > > *** 129,134
> > > > > > --- 129,144
> > > > > >else if (TREE_CODE (t) == VAR_DECL&&decl_function_context
> > > > > > (t)
> > > > > > &&!TREE_STATIC (t))
> > > > > > return false;
> > > > > > + /* If this is a decl generated for block local externs for
> > > > > > + debug info generation, stream it unshared alongside
> > > > > > BLOCK_VARS.
> > > > > > */
> > > > > > + else if (VAR_OR_FUNCTION_DECL_P (t)
> > > > > > + /* ??? The following tests are a literal match on what
> > > > > > + c-decl.c:pop_scope does. */
> > > > >
> > > > > Factor it into a common routine then?
> > > >
> > > > pop_scope of course _sets_ the values that way, it doesn't test them.
> > >
> > > Yes. I meant factoring, so future users have something to call, instead
> > > of
> > > three seemingly random flag checks. pop_scope could also be calling some
> > > complementary setter instead of doing the seemingly random flag setting.
> >
> > I don't see a good way to factor out the flags setting. Factoring out
> > the copying maybe. But well... we can do that when a 2nd user
> > comes along?
>
> The problem is that the 2nd user will cut-n-paste from this one. However, if
> you find adding a little function too strenuous, I guess it's not too big a
> deal.
Testing this kind of patches turns out to be quite time-consuming
(I do a LTO bootstrap and two SPEC 2k6 builds (-g and -g0)), so
yeah ...
The following is what I ended up LTO bootstrapping (finished ok),
testing is still in progress, as is SPEC 2k6 build.
It fixes PR49945 in a similar way, by streaming VLA types locally as well.
I'm going to apply it tomorrow, when full testing hopefully finished
Thanks,
Richard.
2011-12-08 Richard Guenther
PR lto/48437
PR lto/49945
* lto-streamer-out.c (tree_is_indexable): Exclude block-local
extern declarations.
PR lto/48437
* gcc.dg/lto/20111207-2_0.c: New testcase.
* gcc.dg/guality/pr48437.c: Likewise.
Index: gcc/lto-streamer-out.c
===
*** gcc/lto-streamer-out.c (revision 182081)
--- gcc/lto-streamer-out.c (working copy)
*** tree_is_indexable (tree t)
*** 129,134
--- 129,147
else if (TREE_CODE (t) == VAR_DECL && decl_function_context (t)
&& !TREE_STATIC (t))
return false;
+ /* If this is a decl generated for block local externs for
+ debug info generation, stream it unshared alongside BLOCK_VARS. */
+ else if (VAR_OR_FUNCTION_DECL_P (t)
+ /* ??? The following tests are a literal match on what
+ c-decl.c:pop_scope does. */
+ && TREE_PUBLIC (t)
+ && DECL_EXTERNAL (t)
+ && DECL_CONTEXT (t)
+ && TREE_CODE (DECL_CONTEXT (t)) == FUNCTION_DECL)
+ return false;
+ else if (TYPE_P (t)
+ && variably_modified_type_p (t, NULL_TREE))
+ return false;
else
return (TYPE_P (t) || DECL_P (t) || TREE_CODE (t) == SSA_NAME);
}
Index: gcc/testsuite/gcc.dg/lto/20111207-2_0.c
===
*** gcc/testsuite/gcc.dg/lto/20111207-2_0.c (revision 0)
--- gcc/testsuite/gcc.dg/lto/20111207-2_0.c (revision 0)
***
*** 0
--- 1,17
+ /* { dg-lto-do run } */
+
+ int
+ test (void)
+ {
+ int f (void);
+ return 0;
+ }
+
+ int
+ main (void)
+ {
+ int f (void);
+ int test (void);
+
+ return test ();
+ }
Index: gcc/testsuite/gcc.dg/guality/pr48437.c
===
*** gcc/testsuite/gcc.dg/guality/pr48437.c (revision 0)
--- gcc/testsuite/gcc.dg/guality/pr48437.c (revision 0)
***
*** 0
--- 1,17
+ /* PR lto/48437 */
+ /* { dg-do run } */
+ /* { dg-options "-g" } */
+
+ #include "../nop.h"
+
+ int i __attribute__((used));
+ int main()
+ {
+ volatile int i;
+ for (i = 3; i < 7; ++i)
+ {
+ extern int i;
+ asm volatile (NOP : : : "memory"); /* { dg-final { gdb-test 14 "i" "0"
} } */
+ }
+ return 0;
+ }
Re: [PATCH][LTO] Fix PR48437
On 12/07/11 10:54, Richard Guenther wrote: On Wed, 7 Dec 2011, Diego Novillo wrote: On 12/07/11 10:46, Richard Guenther wrote: On Wed, 7 Dec 2011, Diego Novillo wrote: On 12/07/11 09:52, Richard Guenther wrote: Index: gcc/lto-streamer-out.c === *** gcc/lto-streamer-out.c (revision 182081) --- gcc/lto-streamer-out.c (working copy) *** tree_is_indexable (tree t) *** 129,134 --- 129,144 else if (TREE_CODE (t) == VAR_DECL&&decl_function_context (t) &&!TREE_STATIC (t)) return false; + /* If this is a decl generated for block local externs for + debug info generation, stream it unshared alongside BLOCK_VARS. */ + else if (VAR_OR_FUNCTION_DECL_P (t) + /* ??? The following tests are a literal match on what + c-decl.c:pop_scope does. */ Factor it into a common routine then? pop_scope of course _sets_ the values that way, it doesn't test them. Yes. I meant factoring, so future users have something to call, instead of three seemingly random flag checks. pop_scope could also be calling some complementary setter instead of doing the seemingly random flag setting. I don't see a good way to factor out the flags setting. Factoring out the copying maybe. But well... we can do that when a 2nd user comes along? The problem is that the 2nd user will cut-n-paste from this one. However, if you find adding a little function too strenuous, I guess it's not too big a deal. Diego.
Re: [PATCH][LTO] Fix PR48437
On Wed, 7 Dec 2011, Diego Novillo wrote: > On 12/07/11 10:46, Richard Guenther wrote: > > On Wed, 7 Dec 2011, Diego Novillo wrote: > > > > > On 12/07/11 09:52, Richard Guenther wrote: > > > > > > > Index: gcc/lto-streamer-out.c > > > > === > > > > *** gcc/lto-streamer-out.c (revision 182081) > > > > --- gcc/lto-streamer-out.c (working copy) > > > > *** tree_is_indexable (tree t) > > > > *** 129,134 > > > > --- 129,144 > > > > else if (TREE_CODE (t) == VAR_DECL&& decl_function_context (t) > > > > && !TREE_STATIC (t)) > > > > return false; > > > > + /* If this is a decl generated for block local externs for > > > > + debug info generation, stream it unshared alongside BLOCK_VARS. > > > > */ > > > > + else if (VAR_OR_FUNCTION_DECL_P (t) > > > > + /* ??? The following tests are a literal match on what > > > > + c-decl.c:pop_scope does. */ > > > > > > Factor it into a common routine then? > > > > pop_scope of course _sets_ the values that way, it doesn't test them. > > Yes. I meant factoring, so future users have something to call, instead of > three seemingly random flag checks. pop_scope could also be calling some > complementary setter instead of doing the seemingly random flag setting. I don't see a good way to factor out the flags setting. Factoring out the copying maybe. But well... we can do that when a 2nd user comes along? Richard.
Re: [PATCH][LTO] Fix PR48437
On 12/07/11 10:46, Richard Guenther wrote: On Wed, 7 Dec 2011, Diego Novillo wrote: On 12/07/11 09:52, Richard Guenther wrote: Index: gcc/lto-streamer-out.c === *** gcc/lto-streamer-out.c (revision 182081) --- gcc/lto-streamer-out.c (working copy) *** tree_is_indexable (tree t) *** 129,134 --- 129,144 else if (TREE_CODE (t) == VAR_DECL&& decl_function_context (t) && !TREE_STATIC (t)) return false; + /* If this is a decl generated for block local externs for + debug info generation, stream it unshared alongside BLOCK_VARS. */ + else if (VAR_OR_FUNCTION_DECL_P (t) + /* ??? The following tests are a literal match on what + c-decl.c:pop_scope does. */ Factor it into a common routine then? pop_scope of course _sets_ the values that way, it doesn't test them. Yes. I meant factoring, so future users have something to call, instead of three seemingly random flag checks. pop_scope could also be calling some complementary setter instead of doing the seemingly random flag setting. Diego.
Re: [PATCH][LTO] Fix PR48437
On Wed, 7 Dec 2011, Diego Novillo wrote: > On 12/07/11 09:52, Richard Guenther wrote: > > > Index: gcc/lto-streamer-out.c > > === > > *** gcc/lto-streamer-out.c (revision 182081) > > --- gcc/lto-streamer-out.c (working copy) > > *** tree_is_indexable (tree t) > > *** 129,134 > > --- 129,144 > > else if (TREE_CODE (t) == VAR_DECL&& decl_function_context (t) > > && !TREE_STATIC (t)) > >return false; > > + /* If this is a decl generated for block local externs for > > + debug info generation, stream it unshared alongside BLOCK_VARS. */ > > + else if (VAR_OR_FUNCTION_DECL_P (t) > > + /* ??? The following tests are a literal match on what > > + c-decl.c:pop_scope does. */ > > Factor it into a common routine then? pop_scope of course _sets_ the values that way, it doesn't test them. Richard.
Re: [PATCH][LTO] Fix PR48437
On 12/07/11 09:52, Richard Guenther wrote: Index: gcc/lto-streamer-out.c === *** gcc/lto-streamer-out.c (revision 182081) --- gcc/lto-streamer-out.c (working copy) *** tree_is_indexable (tree t) *** 129,134 --- 129,144 else if (TREE_CODE (t) == VAR_DECL&& decl_function_context (t) && !TREE_STATIC (t)) return false; + /* If this is a decl generated for block local externs for + debug info generation, stream it unshared alongside BLOCK_VARS. */ + else if (VAR_OR_FUNCTION_DECL_P (t) + /* ??? The following tests are a literal match on what + c-decl.c:pop_scope does. */ Factor it into a common routine then? Diego.
Re: [PATCH][LTO] Fix PR48437
On Wed, 7 Dec 2011, Jakub Jelinek wrote:
> On Wed, Dec 07, 2011 at 03:52:35PM +0100, Richard Guenther wrote:
> > *** gcc/testsuite/gcc.dg/guality/pr48437.c (revision 0)
> > --- gcc/testsuite/gcc.dg/guality/pr48437.c (revision 0)
> > ***
> > *** 0
> > --- 1,15
> > + /* PR lto/48437 */
> > + /* { dg-do run } */
> > + /* { dg-options "-g" } */
> > +
> > + int i __attribute__((used));
> > + int main()
> > + {
> > + volatile int i;
> > + for (i = 3; i < 7; ++i)
> > + {
> > + extern int i; /* { dg-final { gdb-test 7 "i" "0" } } */
> > + asm volatile ("" : : : "memory");
>
> Do you really want to test it on line 7 ({ of after main())?
Oops - so much for adding the /* dg */ stuff late ...
> I'd expect you want to look at it on the line where asm volatile is,
> and make sure you can put a breakpoint on it:
> #include "../nop.h"
> early and use NOP instead of "" as the asm pattern.
Changed.
Thanks,
Richard.
Re: [PATCH][LTO] Fix PR48437
On Wed, Dec 07, 2011 at 03:52:35PM +0100, Richard Guenther wrote:
> *** gcc/testsuite/gcc.dg/guality/pr48437.c(revision 0)
> --- gcc/testsuite/gcc.dg/guality/pr48437.c(revision 0)
> ***
> *** 0
> --- 1,15
> + /* PR lto/48437 */
> + /* { dg-do run } */
> + /* { dg-options "-g" } */
> +
> + int i __attribute__((used));
> + int main()
> + {
> + volatile int i;
> + for (i = 3; i < 7; ++i)
> + {
> + extern int i; /* { dg-final { gdb-test 7 "i" "0" } } */
> + asm volatile ("" : : : "memory");
Do you really want to test it on line 7 ({ of after main())?
I'd expect you want to look at it on the line where asm volatile is,
and make sure you can put a breakpoint on it:
#include "../nop.h"
early and use NOP instead of "" as the asm pattern.
Jakub
[PATCH][LTO] Fix PR48437
I'm testing the following patch for PR48437 - the C frontend generates
decl copies for placing them in BLOCK_VARS for
int
test (void)
{
extern int f (void);
return 0;
}
we currently end up putting those into the decl merging machinery,
causing DECL_CHAIN re-use and subsequent crashes. By not indexing
them we make sure to keep them separate.
LTO bootstrap & regtest pending on x86_64-unknown-linux-gnu, ok?
[Similar fix has to be employed for VLA types]
Thanks,
Richard.
2011-12-07 Richard Guenther
PR lto/48437
* lto-streamer-out.c (tree_is_indexable): Exclude block-local
extern declarations.
* gcc.dg/lto/20111207-2_0.c: New testcase.
* gcc.dg/guality/pr48437.c: Likewise.
Index: gcc/lto-streamer-out.c
===
*** gcc/lto-streamer-out.c (revision 182081)
--- gcc/lto-streamer-out.c (working copy)
*** tree_is_indexable (tree t)
*** 129,134
--- 129,144
else if (TREE_CODE (t) == VAR_DECL && decl_function_context (t)
&& !TREE_STATIC (t))
return false;
+ /* If this is a decl generated for block local externs for
+ debug info generation, stream it unshared alongside BLOCK_VARS. */
+ else if (VAR_OR_FUNCTION_DECL_P (t)
+ /* ??? The following tests are a literal match on what
+ c-decl.c:pop_scope does. */
+ && TREE_PUBLIC (t)
+ && DECL_EXTERNAL (t)
+ && DECL_CONTEXT (t)
+ && TREE_CODE (DECL_CONTEXT (t)) == FUNCTION_DECL)
+ return false;
else
return (TYPE_P (t) || DECL_P (t) || TREE_CODE (t) == SSA_NAME);
}
Index: gcc/testsuite/gcc.dg/lto/20111207-2_0.c
===
*** gcc/testsuite/gcc.dg/lto/20111207-2_0.c (revision 0)
--- gcc/testsuite/gcc.dg/lto/20111207-2_0.c (revision 0)
***
*** 0
--- 1,17
+ /* { dg-lto-do run } */
+
+ int
+ test (void)
+ {
+ int f (void);
+ return 0;
+ }
+
+ int
+ main (void)
+ {
+ int f (void);
+ int test (void);
+
+ return test ();
+ }
Index: gcc/testsuite/gcc.dg/guality/pr48437.c
===
*** gcc/testsuite/gcc.dg/guality/pr48437.c (revision 0)
--- gcc/testsuite/gcc.dg/guality/pr48437.c (revision 0)
***
*** 0
--- 1,15
+ /* PR lto/48437 */
+ /* { dg-do run } */
+ /* { dg-options "-g" } */
+
+ int i __attribute__((used));
+ int main()
+ {
+ volatile int i;
+ for (i = 3; i < 7; ++i)
+ {
+ extern int i; /* { dg-final { gdb-test 7 "i" "0" } } */
+ asm volatile ("" : : : "memory");
+ }
+ return 0;
+ }
