| Message ID | 20141126123545.GB40854@msticlxl57.ims.intel.com |
|---|---|
| State | New |
| Headers | show |
> On 25 Nov 15:03, Ilya Enkovich wrote: > > 2014-11-25 14:11 GMT+03:00 Richard Biener <richard.guenther@gmail.com>: > > > On Tue, Nov 25, 2014 at 11:19 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > > > > > > Ok, then it's get_for_asmname (). That said - the above loops look > > > bogus to me. Honza - any better ideas? > > > > get_for_asmname () returns the first element in a chain of nodes with > > the same asm name. May I rely on the order of nodes in this chain? > > Probably use ASSEMBLER_NAME as a key in chkp_static_var_bounds hash? > > > > Thanks, > > Ilya > > > > > > > > Richard. > > > > > A variant with var's ASSEMBLER_NAME as a key works fine. Instrumented bootstrap passes. OK for trunk? It is possible to have two different assembler name for same resulting symbol. Why you simply don't use symtab nodes as keys? Honza > > Thanks, > Ilya > -- > gcc/ > > 2014-11-26 Ilya Enkovich <ilya.enkovich@intel.com> > > PR bootstrap/63995 > * tree-chkp.c (chkp_make_static_bounds): Share bounds var > between nodes sharing assembler name. > > gcc/testsuite > > 2014-11-26 Ilya Enkovich <ilya.enkovich@intel.com> > > PR bootstrap/63995 > * g++.dg/dg.exp: Add mpx-dg.exp. > * g++.dg/pr63995-1.C: New. > > > diff --git a/gcc/testsuite/g++.dg/dg.exp b/gcc/testsuite/g++.dg/dg.exp > index 14beae1..44eab0c 100644 > --- a/gcc/testsuite/g++.dg/dg.exp > +++ b/gcc/testsuite/g++.dg/dg.exp > @@ -18,6 +18,7 @@ > > # Load support procs. > load_lib g++-dg.exp > +load_lib mpx-dg.exp > > # If a testcase doesn't have special options, use these. > global DEFAULT_CXXFLAGS > diff --git a/gcc/testsuite/g++.dg/pr63995-1.C b/gcc/testsuite/g++.dg/pr63995-1.C > new file mode 100644 > index 0000000..82e7606 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/pr63995-1.C > @@ -0,0 +1,16 @@ > +/* { dg-do compile { target i?86-*-* x86_64-*-* } } */ > +/* { dg-require-effective-target mpx } */ > +/* { dg-options "-O2 -g -fcheck-pointer-bounds -mmpx" } */ > + > +int test1 (int i) > +{ > + extern const int arr[10]; > + return arr[i]; > +} > + > +extern const int arr[10]; > + > +int test2 (int i) > +{ > + return arr[i]; > +} > diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c > index 3e38691..924cb71 100644 > --- a/gcc/tree-chkp.c > +++ b/gcc/tree-chkp.c > @@ -2727,9 +2727,23 @@ chkp_make_static_bounds (tree obj) > /* First check if we already have required var. */ > if (chkp_static_var_bounds) > { > - slot = chkp_static_var_bounds->get (obj); > - if (slot) > - return *slot; > + /* For vars we use assembler name as a key in > + chkp_static_var_bounds map. It allows to > + avoid duplicating bound vars for decls > + sharing assembler name. */ > + if (TREE_CODE (obj) == VAR_DECL) > + { > + tree name = DECL_ASSEMBLER_NAME (obj); > + slot = chkp_static_var_bounds->get (name); > + if (slot) > + return *slot; > + } > + else > + { > + slot = chkp_static_var_bounds->get (obj); > + if (slot) > + return *slot; > + } > } > > /* Build decl for bounds var. */ > @@ -2793,7 +2807,13 @@ chkp_make_static_bounds (tree obj) > if (!chkp_static_var_bounds) > chkp_static_var_bounds = new hash_map<tree, tree>; > > - chkp_static_var_bounds->put (obj, bnd_var); > + if (TREE_CODE (obj) == VAR_DECL) > + { > + tree name = DECL_ASSEMBLER_NAME (obj); > + chkp_static_var_bounds->put (name, bnd_var); > + } > + else > + chkp_static_var_bounds->put (obj, bnd_var); > > return bnd_var; > }
2014-11-26 19:07 GMT+03:00 Jan Hubicka <hubicka@ucw.cz>: >> On 25 Nov 15:03, Ilya Enkovich wrote: >> > 2014-11-25 14:11 GMT+03:00 Richard Biener <richard.guenther@gmail.com>: >> > > On Tue, Nov 25, 2014 at 11:19 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >> > > >> > > Ok, then it's get_for_asmname (). That said - the above loops look >> > > bogus to me. Honza - any better ideas? >> > >> > get_for_asmname () returns the first element in a chain of nodes with >> > the same asm name. May I rely on the order of nodes in this chain? >> > Probably use ASSEMBLER_NAME as a key in chkp_static_var_bounds hash? >> > >> > Thanks, >> > Ilya >> > >> > > >> > > Richard. >> > > >> >> A variant with var's ASSEMBLER_NAME as a key works fine. Instrumented bootstrap passes. OK for trunk? > > It is possible to have two different assembler name for same resulting symbol. What do you mean? DECL_ASSEMBLER_NAME is changed during compilation? Or transparent alias chain? > Why you simply don't use symtab nodes as keys? I cannot use symtab node because I have two different nodes for symbols with the same assembler name. I suppose with LTO these symbols would be merged. But having two var decls and two symtab nodes I need to create and emit only one corresponding bounds symbol. Ilya > > Honza >> >> Thanks, >> Ilya >> -- >> gcc/ >> >> 2014-11-26 Ilya Enkovich <ilya.enkovich@intel.com> >> >> PR bootstrap/63995 >> * tree-chkp.c (chkp_make_static_bounds): Share bounds var >> between nodes sharing assembler name. >> >> gcc/testsuite >> >> 2014-11-26 Ilya Enkovich <ilya.enkovich@intel.com> >> >> PR bootstrap/63995 >> * g++.dg/dg.exp: Add mpx-dg.exp. >> * g++.dg/pr63995-1.C: New. >> >> >> diff --git a/gcc/testsuite/g++.dg/dg.exp b/gcc/testsuite/g++.dg/dg.exp >> index 14beae1..44eab0c 100644 >> --- a/gcc/testsuite/g++.dg/dg.exp >> +++ b/gcc/testsuite/g++.dg/dg.exp >> @@ -18,6 +18,7 @@ >> >> # Load support procs. >> load_lib g++-dg.exp >> +load_lib mpx-dg.exp >> >> # If a testcase doesn't have special options, use these. >> global DEFAULT_CXXFLAGS >> diff --git a/gcc/testsuite/g++.dg/pr63995-1.C b/gcc/testsuite/g++.dg/pr63995-1.C >> new file mode 100644 >> index 0000000..82e7606 >> --- /dev/null >> +++ b/gcc/testsuite/g++.dg/pr63995-1.C >> @@ -0,0 +1,16 @@ >> +/* { dg-do compile { target i?86-*-* x86_64-*-* } } */ >> +/* { dg-require-effective-target mpx } */ >> +/* { dg-options "-O2 -g -fcheck-pointer-bounds -mmpx" } */ >> + >> +int test1 (int i) >> +{ >> + extern const int arr[10]; >> + return arr[i]; >> +} >> + >> +extern const int arr[10]; >> + >> +int test2 (int i) >> +{ >> + return arr[i]; >> +} >> diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c >> index 3e38691..924cb71 100644 >> --- a/gcc/tree-chkp.c >> +++ b/gcc/tree-chkp.c >> @@ -2727,9 +2727,23 @@ chkp_make_static_bounds (tree obj) >> /* First check if we already have required var. */ >> if (chkp_static_var_bounds) >> { >> - slot = chkp_static_var_bounds->get (obj); >> - if (slot) >> - return *slot; >> + /* For vars we use assembler name as a key in >> + chkp_static_var_bounds map. It allows to >> + avoid duplicating bound vars for decls >> + sharing assembler name. */ >> + if (TREE_CODE (obj) == VAR_DECL) >> + { >> + tree name = DECL_ASSEMBLER_NAME (obj); >> + slot = chkp_static_var_bounds->get (name); >> + if (slot) >> + return *slot; >> + } >> + else >> + { >> + slot = chkp_static_var_bounds->get (obj); >> + if (slot) >> + return *slot; >> + } >> } >> >> /* Build decl for bounds var. */ >> @@ -2793,7 +2807,13 @@ chkp_make_static_bounds (tree obj) >> if (!chkp_static_var_bounds) >> chkp_static_var_bounds = new hash_map<tree, tree>; >> >> - chkp_static_var_bounds->put (obj, bnd_var); >> + if (TREE_CODE (obj) == VAR_DECL) >> + { >> + tree name = DECL_ASSEMBLER_NAME (obj); >> + chkp_static_var_bounds->put (name, bnd_var); >> + } >> + else >> + chkp_static_var_bounds->put (obj, bnd_var); >> >> return bnd_var; >> }
Ping 2014-11-26 15:35 GMT+03:00 Ilya Enkovich <enkovich.gnu@gmail.com>: > On 25 Nov 15:03, Ilya Enkovich wrote: >> 2014-11-25 14:11 GMT+03:00 Richard Biener <richard.guenther@gmail.com>: >> > On Tue, Nov 25, 2014 at 11:19 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >> > >> > Ok, then it's get_for_asmname (). That said - the above loops look >> > bogus to me. Honza - any better ideas? >> >> get_for_asmname () returns the first element in a chain of nodes with >> the same asm name. May I rely on the order of nodes in this chain? >> Probably use ASSEMBLER_NAME as a key in chkp_static_var_bounds hash? >> >> Thanks, >> Ilya >> >> > >> > Richard. >> > > > A variant with var's ASSEMBLER_NAME as a key works fine. Instrumented bootstrap passes. OK for trunk? > > Thanks, > Ilya > -- > gcc/ > > 2014-11-26 Ilya Enkovich <ilya.enkovich@intel.com> > > PR bootstrap/63995 > * tree-chkp.c (chkp_make_static_bounds): Share bounds var > between nodes sharing assembler name. > > gcc/testsuite > > 2014-11-26 Ilya Enkovich <ilya.enkovich@intel.com> > > PR bootstrap/63995 > * g++.dg/dg.exp: Add mpx-dg.exp. > * g++.dg/pr63995-1.C: New. > > > diff --git a/gcc/testsuite/g++.dg/dg.exp b/gcc/testsuite/g++.dg/dg.exp > index 14beae1..44eab0c 100644 > --- a/gcc/testsuite/g++.dg/dg.exp > +++ b/gcc/testsuite/g++.dg/dg.exp > @@ -18,6 +18,7 @@ > > # Load support procs. > load_lib g++-dg.exp > +load_lib mpx-dg.exp > > # If a testcase doesn't have special options, use these. > global DEFAULT_CXXFLAGS > diff --git a/gcc/testsuite/g++.dg/pr63995-1.C b/gcc/testsuite/g++.dg/pr63995-1.C > new file mode 100644 > index 0000000..82e7606 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/pr63995-1.C > @@ -0,0 +1,16 @@ > +/* { dg-do compile { target i?86-*-* x86_64-*-* } } */ > +/* { dg-require-effective-target mpx } */ > +/* { dg-options "-O2 -g -fcheck-pointer-bounds -mmpx" } */ > + > +int test1 (int i) > +{ > + extern const int arr[10]; > + return arr[i]; > +} > + > +extern const int arr[10]; > + > +int test2 (int i) > +{ > + return arr[i]; > +} > diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c > index 3e38691..924cb71 100644 > --- a/gcc/tree-chkp.c > +++ b/gcc/tree-chkp.c > @@ -2727,9 +2727,23 @@ chkp_make_static_bounds (tree obj) > /* First check if we already have required var. */ > if (chkp_static_var_bounds) > { > - slot = chkp_static_var_bounds->get (obj); > - if (slot) > - return *slot; > + /* For vars we use assembler name as a key in > + chkp_static_var_bounds map. It allows to > + avoid duplicating bound vars for decls > + sharing assembler name. */ > + if (TREE_CODE (obj) == VAR_DECL) > + { > + tree name = DECL_ASSEMBLER_NAME (obj); > + slot = chkp_static_var_bounds->get (name); > + if (slot) > + return *slot; > + } > + else > + { > + slot = chkp_static_var_bounds->get (obj); > + if (slot) > + return *slot; > + } > } > > /* Build decl for bounds var. */ > @@ -2793,7 +2807,13 @@ chkp_make_static_bounds (tree obj) > if (!chkp_static_var_bounds) > chkp_static_var_bounds = new hash_map<tree, tree>; > > - chkp_static_var_bounds->put (obj, bnd_var); > + if (TREE_CODE (obj) == VAR_DECL) > + { > + tree name = DECL_ASSEMBLER_NAME (obj); > + chkp_static_var_bounds->put (name, bnd_var); > + } > + else > + chkp_static_var_bounds->put (obj, bnd_var); > > return bnd_var; > }
On 12/08/14 04:00, Ilya Enkovich wrote: >> 2014-11-26 Ilya Enkovich <ilya.enkovich@intel.com> >> >> PR bootstrap/63995 >> * tree-chkp.c (chkp_make_static_bounds): Share bounds var >> between nodes sharing assembler name. >> >> gcc/testsuite >> >> 2014-11-26 Ilya Enkovich <ilya.enkovich@intel.com> >> >> PR bootstrap/63995 >> * g++.dg/dg.exp: Add mpx-dg.exp. >> * g++.dg/pr63995-1.C: New. Code seems fine. Not sure how the testcase is actually testing that you're sharing bounds though. The test AFAICT just verifies that it can be compiled and doesn't checkout the output in any way. Or is it the case that the test will ICE or segfault without your fix? Jeff
2014-12-09 0:00 GMT+03:00 Jeff Law <law@redhat.com>: > On 12/08/14 04:00, Ilya Enkovich wrote: >>> >>> 2014-11-26 Ilya Enkovich <ilya.enkovich@intel.com> >>> >>> PR bootstrap/63995 >>> * tree-chkp.c (chkp_make_static_bounds): Share bounds var >>> between nodes sharing assembler name. >>> >>> gcc/testsuite >>> >>> 2014-11-26 Ilya Enkovich <ilya.enkovich@intel.com> >>> >>> PR bootstrap/63995 >>> * g++.dg/dg.exp: Add mpx-dg.exp. >>> * g++.dg/pr63995-1.C: New. > > Code seems fine. Not sure how the testcase is actually testing that you're > sharing bounds though. The test AFAICT just verifies that it can be > compiled and doesn't checkout the output in any way. Or is it the case that > the test will ICE or segfault without your fix? Without this fix compiler emits two symbols with the same name and assembler complains. Something like that: /tmp/cceHGHa1.s: Assembler messages: /tmp/cceHGHa1.s:12080: Error: symbol `__chkp_bounds_of_mode_inner' is already defined /tmp/cceHGHa1.s:12086: Error: symbol `__chkp_bounds_of_mode_nunits' is already defined Thanks, Ilya > > > Jeff > >
On 12/08/14 14:12, Ilya Enkovich wrote: > 2014-12-09 0:00 GMT+03:00 Jeff Law <law@redhat.com>: >> On 12/08/14 04:00, Ilya Enkovich wrote: >>>> >>>> 2014-11-26 Ilya Enkovich <ilya.enkovich@intel.com> >>>> >>>> PR bootstrap/63995 >>>> * tree-chkp.c (chkp_make_static_bounds): Share bounds var >>>> between nodes sharing assembler name. >>>> >>>> gcc/testsuite >>>> >>>> 2014-11-26 Ilya Enkovich <ilya.enkovich@intel.com> >>>> >>>> PR bootstrap/63995 >>>> * g++.dg/dg.exp: Add mpx-dg.exp. >>>> * g++.dg/pr63995-1.C: New. >> >> Code seems fine. Not sure how the testcase is actually testing that you're >> sharing bounds though. The test AFAICT just verifies that it can be >> compiled and doesn't checkout the output in any way. Or is it the case that >> the test will ICE or segfault without your fix? > > Without this fix compiler emits two symbols with the same name and > assembler complains. Something like that: > > /tmp/cceHGHa1.s: Assembler messages: > /tmp/cceHGHa1.s:12080: Error: symbol `__chkp_bounds_of_mode_inner' is > already defined > /tmp/cceHGHa1.s:12086: Error: symbol `__chkp_bounds_of_mode_nunits' is > already defined Ah OK. Ok for the trunk. Thanks for your patience, Jeff
diff --git a/gcc/testsuite/g++.dg/dg.exp b/gcc/testsuite/g++.dg/dg.exp index 14beae1..44eab0c 100644 --- a/gcc/testsuite/g++.dg/dg.exp +++ b/gcc/testsuite/g++.dg/dg.exp @@ -18,6 +18,7 @@ # Load support procs. load_lib g++-dg.exp +load_lib mpx-dg.exp # If a testcase doesn't have special options, use these. global DEFAULT_CXXFLAGS diff --git a/gcc/testsuite/g++.dg/pr63995-1.C b/gcc/testsuite/g++.dg/pr63995-1.C new file mode 100644 index 0000000..82e7606 --- /dev/null +++ b/gcc/testsuite/g++.dg/pr63995-1.C @@ -0,0 +1,16 @@ +/* { dg-do compile { target i?86-*-* x86_64-*-* } } */ +/* { dg-require-effective-target mpx } */ +/* { dg-options "-O2 -g -fcheck-pointer-bounds -mmpx" } */ + +int test1 (int i) +{ + extern const int arr[10]; + return arr[i]; +} + +extern const int arr[10]; + +int test2 (int i) +{ + return arr[i]; +} diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c index 3e38691..924cb71 100644 --- a/gcc/tree-chkp.c +++ b/gcc/tree-chkp.c @@ -2727,9 +2727,23 @@ chkp_make_static_bounds (tree obj) /* First check if we already have required var. */ if (chkp_static_var_bounds) { - slot = chkp_static_var_bounds->get (obj); - if (slot) - return *slot; + /* For vars we use assembler name as a key in + chkp_static_var_bounds map. It allows to + avoid duplicating bound vars for decls + sharing assembler name. */ + if (TREE_CODE (obj) == VAR_DECL) + { + tree name = DECL_ASSEMBLER_NAME (obj); + slot = chkp_static_var_bounds->get (name); + if (slot) + return *slot; + } + else + { + slot = chkp_static_var_bounds->get (obj); + if (slot) + return *slot; + } } /* Build decl for bounds var. */ @@ -2793,7 +2807,13 @@ chkp_make_static_bounds (tree obj) if (!chkp_static_var_bounds) chkp_static_var_bounds = new hash_map<tree, tree>; - chkp_static_var_bounds->put (obj, bnd_var); + if (TREE_CODE (obj) == VAR_DECL) + { + tree name = DECL_ASSEMBLER_NAME (obj); + chkp_static_var_bounds->put (name, bnd_var); + } + else + chkp_static_var_bounds->put (obj, bnd_var); return bnd_var; }