| Message ID | 53AC2386.3030309@suse.cz |
|---|---|
| State | New |
| Headers | show |
On Thu, Jun 26, 2014 at 3:43 PM, Martin Liška <mliska@suse.cz> wrote: > > On 06/26/2014 03:20 PM, Richard Biener wrote: >> >> On Thu, Jun 26, 2014 at 3:01 PM, Martin Liška <mliska@suse.cz> wrote: >>> >>> Hello, >>> I encountered similar issue to PR ipa/61462 where location_t locus = >>> gimple_location (e->call_stmt) is called for e->call_stmt == NULL >>> (Firefox >>> with -flto -fdump-ipa-devirt). So that, I decided to introduce new >>> function >>> that is called for all potentially unsafe locations. I am wondering if a >>> newly added function can be added in more seamless way (without playing >>> with >>> va_list and ATTRIBUTE_PRINTF stuff)? >>> >>> Bootstrapped and regtested on x86_64-unknown-linux-gnu. >> >> Hmm, I don't like that very much - dump_printf_loc_for_stmt still implies >> stmt is not NULL. So you could have "fixed" gimple_location as well. >> I suppose dump_printf_loc already does sth sane with UNKNOWN_LOCATION. >> >> Richard. > > Hi, > you are right that it is quite complex change. > > Do you mean this one line change can be sufficient ? > diff --git a/gcc/gimple.h b/gcc/gimple.h > index ceefbc0..954195e 100644 > --- a/gcc/gimple.h > +++ b/gcc/gimple.h > @@ -1498,7 +1498,7 @@ gimple_set_block (gimple g, tree block) > static inline location_t > gimple_location (const_gimple g) > { > - return g->location; > + return g ? g->location : UNKNOWN_LOCATION; > } > > /* Return pointer to location information for statement G. */ > > I will double-check if it solves the problem ;) Well yes - it is of course similar broken in spirit but at least a lot simpler ;) I'd put a comment there why we do check g for NULL. Thanks, Richard. > Martin > > >> >>> Thanks, >>> Martin >>> >>> ChangeLog: >>> >>> 2014-06-26 Martin Liska <mliska@suse.cz> >>> >>> * include/ansidecl.h: New collection of >>> ATTRIBUTE_NULL_PRINTF_X_0 >>> defined. >>> >>> gcc/ChangeLog: >>> >>> 2014-06-26 Martin Liska <mliska@suse.cz> >>> >>> * dumpfile.h: New function dump_printf_loc_for_stmt. >>> * dumpfile.c: Implementation added. >>> (dump_vprintf): New function.i >>> * cgraphunit.c: dump_printf_loc_for_stmt usage replaces >>> dump_printf_loc. >>> * gimple-fold.c: Likewise. >>> * ipa-devirt.c: Likewise. >>> * ipa-prop.c: Likewise. >>> * ipa.c: Likewise. >>> * tree-ssa-pre.c: Likewise. >>> >>> >>> >>> >
On Thu, Jun 26, 2014 at 04:10:03PM +0200, Richard Biener wrote: > On Thu, Jun 26, 2014 at 3:43 PM, Martin Liška <mliska@suse.cz> wrote: > > > > On 06/26/2014 03:20 PM, Richard Biener wrote: > >> > >> On Thu, Jun 26, 2014 at 3:01 PM, Martin Liška <mliska@suse.cz> wrote: > >>> > >>> Hello, > >>> I encountered similar issue to PR ipa/61462 where location_t locus = > >>> gimple_location (e->call_stmt) is called for e->call_stmt == NULL > >>> (Firefox > >>> with -flto -fdump-ipa-devirt). So that, I decided to introduce new > >>> function > >>> that is called for all potentially unsafe locations. I am wondering if a > >>> newly added function can be added in more seamless way (without playing > >>> with > >>> va_list and ATTRIBUTE_PRINTF stuff)? > >>> > >>> Bootstrapped and regtested on x86_64-unknown-linux-gnu. > >> > >> Hmm, I don't like that very much - dump_printf_loc_for_stmt still implies > >> stmt is not NULL. So you could have "fixed" gimple_location as well. > >> I suppose dump_printf_loc already does sth sane with UNKNOWN_LOCATION. > >> > >> Richard. > > > > Hi, > > you are right that it is quite complex change. > > > > Do you mean this one line change can be sufficient ? > > diff --git a/gcc/gimple.h b/gcc/gimple.h > > index ceefbc0..954195e 100644 > > --- a/gcc/gimple.h > > +++ b/gcc/gimple.h > > @@ -1498,7 +1498,7 @@ gimple_set_block (gimple g, tree block) > > static inline location_t > > gimple_location (const_gimple g) > > { > > - return g->location; > > + return g ? g->location : UNKNOWN_LOCATION; > > } > > > > /* Return pointer to location information for statement G. */ > > > > I will double-check if it solves the problem ;) > > Well yes - it is of course similar broken in spirit but at least a lot > simpler ;) I'd put a comment there why we do check g for NULL. But it increases overhead, there are hundreds of gimple_location calls and most of them will never pass NULL. Can't you simply do what you do in the inline here in the couple of spots where the stmt might be NULL? Jakub
On 06/26/2014 04:18 PM, Jakub Jelinek wrote: > On Thu, Jun 26, 2014 at 04:10:03PM +0200, Richard Biener wrote: >> On Thu, Jun 26, 2014 at 3:43 PM, Martin Liška <mliska@suse.cz> wrote: >>> On 06/26/2014 03:20 PM, Richard Biener wrote: >>>> On Thu, Jun 26, 2014 at 3:01 PM, Martin Liška <mliska@suse.cz> wrote: >>>>> Hello, >>>>> I encountered similar issue to PR ipa/61462 where location_t locus = >>>>> gimple_location (e->call_stmt) is called for e->call_stmt == NULL >>>>> (Firefox >>>>> with -flto -fdump-ipa-devirt). So that, I decided to introduce new >>>>> function >>>>> that is called for all potentially unsafe locations. I am wondering if a >>>>> newly added function can be added in more seamless way (without playing >>>>> with >>>>> va_list and ATTRIBUTE_PRINTF stuff)? >>>>> >>>>> Bootstrapped and regtested on x86_64-unknown-linux-gnu. >>>> Hmm, I don't like that very much - dump_printf_loc_for_stmt still implies >>>> stmt is not NULL. So you could have "fixed" gimple_location as well. >>>> I suppose dump_printf_loc already does sth sane with UNKNOWN_LOCATION. >>>> >>>> Richard. >>> Hi, >>> you are right that it is quite complex change. >>> >>> Do you mean this one line change can be sufficient ? >>> diff --git a/gcc/gimple.h b/gcc/gimple.h >>> index ceefbc0..954195e 100644 >>> --- a/gcc/gimple.h >>> +++ b/gcc/gimple.h >>> @@ -1498,7 +1498,7 @@ gimple_set_block (gimple g, tree block) >>> static inline location_t >>> gimple_location (const_gimple g) >>> { >>> - return g->location; >>> + return g ? g->location : UNKNOWN_LOCATION; >>> } >>> >>> /* Return pointer to location information for statement G. */ >>> >>> I will double-check if it solves the problem ;) >> Well yes - it is of course similar broken in spirit but at least a lot >> simpler ;) I'd put a comment there why we do check g for NULL. > But it increases overhead, there are hundreds of gimple_location calls > and most of them will never pass NULL. Can't you simply > do what you do in the inline here in the couple of spots where > the stmt might be NULL? Sure, do you have any suggestion how should be called such function? Suggestion: gimple_location_or_unknown ? Thanks, Martin > > Jakub
On Thu, Jun 26, 2014 at 04:27:49PM +0200, Martin Liška wrote: > >>Well yes - it is of course similar broken in spirit but at least a lot > >>simpler ;) I'd put a comment there why we do check g for NULL. > >But it increases overhead, there are hundreds of gimple_location calls > >and most of them will never pass NULL. Can't you simply > >do what you do in the inline here in the couple of spots where > >the stmt might be NULL? > Sure, do you have any suggestion how should be called such function? > Suggestion: gimple_location_or_unknown ? gimple_location_safe or gimple_safe_location? Jakub
diff --git a/gcc/gimple.h b/gcc/gimple.h index ceefbc0..954195e 100644 --- a/gcc/gimple.h +++ b/gcc/gimple.h @@ -1498,7 +1498,7 @@ gimple_set_block (gimple g, tree block) static inline location_t gimple_location (const_gimple g) { - return g->location; + return g ? g->location : UNKNOWN_LOCATION; } /* Return pointer to location information for statement G. */