| Message ID | 20110105131525.GC24016@kam.mff.cuni.cz |
|---|---|
| State | New |
| Headers | show |
On Wed, Jan 05, 2011 at 02:15:25PM +0100, Jan Hubicka wrote: > + /* Flag_shlib is usually set by finish_options, but we are issing flag_pic issing? > + too late. */ > + if (flag_pic && !flag_pie) > + flag_shlib = 1; > VEC_free (opt_t, heap, opts); > } Jakub
> On Wed, Jan 05, 2011 at 02:15:25PM +0100, Jan Hubicka wrote: > > + /* Flag_shlib is usually set by finish_options, but we are issing flag_pic > > issing? issuing. Fixed in my local copy. Honza > > > + too late. */ > > + if (flag_pic && !flag_pie) > > + flag_shlib = 1; > > VEC_free (opt_t, heap, opts); > > } > > Jakub
On Wed, Jan 5, 2011 at 2:21 PM, Jan Hubicka <hubicka@ucw.cz> wrote: >> On Wed, Jan 05, 2011 at 02:15:25PM +0100, Jan Hubicka wrote: >> > + /* Flag_shlib is usually set by finish_options, but we are issing flag_pic >> >> issing? > issuing. Fixed in my local copy. Hm, I suppose we might want to call finish_options again after re-issuing options instead. Richard. > Honza >> >> > + too late. */ >> > + if (flag_pic && !flag_pie) >> > + flag_shlib = 1; >> > VEC_free (opt_t, heap, opts); >> > } >> >> Jakub >
> On Wed, Jan 5, 2011 at 2:21 PM, Jan Hubicka <hubicka@ucw.cz> wrote: > >> On Wed, Jan 05, 2011 at 02:15:25PM +0100, Jan Hubicka wrote: > >> > + /* Flag_shlib is usually set by finish_options, but we are issing flag_pic > >> > >> issing? > > issuing. Fixed in my local copy. > > Hm, I suppose we might want to call finish_options again after > re-issuing options instead. Hmm, I didn't do that since finish_options seems to contain guard against repeated execution if (!opts->x_flag_opts_finished) { if (opts->x_flag_pie) opts->x_flag_pic = opts->x_flag_pie; if (opts->x_flag_pic && !opts->x_flag_pie) opts->x_flag_shlib = 1; opts->x_flag_opts_finished = false; } only now I noticed that opts_finished is set to false that it is already. This looks like typo. What was original motivation for that thing? I am not sure if we want to finish options again since it has all that tristate handling in it that would need to be saved into opts section. But basically I have no idea, option handling seemed like magic to me all the time and lto opts like a mess... We just need to fix the shlib issue somehow or most of PIC builds with LTO will be broken. Honza > > Richard. > > > Honza > >> > >> > + too late. */ > >> > + if (flag_pic && !flag_pie) > >> > + flag_shlib = 1; > >> > VEC_free (opt_t, heap, opts); > >> > } > >> > >> Jakub > >
2011/1/7 Jan Hubicka <hubicka@ucw.cz>: >> On Wed, Jan 5, 2011 at 2:21 PM, Jan Hubicka <hubicka@ucw.cz> wrote: >> >> On Wed, Jan 05, 2011 at 02:15:25PM +0100, Jan Hubicka wrote: >> >> > + /* Flag_shlib is usually set by finish_options, but we are issing flag_pic >> >> >> >> issing? >> > issuing. Fixed in my local copy. >> >> Hm, I suppose we might want to call finish_options again after >> re-issuing options instead. > > Hmm, I didn't do that since finish_options seems to contain guard against > repeated execution > if (!opts->x_flag_opts_finished) > { > if (opts->x_flag_pie) > opts->x_flag_pic = opts->x_flag_pie; > if (opts->x_flag_pic && !opts->x_flag_pie) > opts->x_flag_shlib = 1; > opts->x_flag_opts_finished = false; > } > only now I noticed that opts_finished is set to false that it is already. This looks like typo. > What was original motivation for that thing? > > I am not sure if we want to finish options again since it has all that tristate handling in it > that would need to be saved into opts section. > But basically I have no idea, option handling seemed like magic to me all the time and lto > opts like a mess... We just need to fix the shlib issue somehow or most of PIC builds with > LTO will be broken. I guess we can sort out things when we fix LTO option handling for real in 4.7. Your original patch is ok. Thanks, Richard. > Honza >> >> Richard. >> >> > Honza >> >> >> >> > + too late. */ >> >> > + if (flag_pic && !flag_pie) >> >> > + flag_shlib = 1; >> >> > VEC_free (opt_t, heap, opts); >> >> > } >> >> >> >> Jakub >> > >
On Fri, 7 Jan 2011, Jan Hubicka wrote: > Hmm, I didn't do that since finish_options seems to contain guard against > repeated execution > if (!opts->x_flag_opts_finished) > { > if (opts->x_flag_pie) > opts->x_flag_pic = opts->x_flag_pie; > if (opts->x_flag_pic && !opts->x_flag_pie) > opts->x_flag_shlib = 1; > opts->x_flag_opts_finished = false; > } > only now I noticed that opts_finished is set to false that it is > already. This looks like typo. That's definitely a mistake. > What was original motivation for that thing? x_flag_opts_finished is meant to be a replacement (avoiding global state) for code that ran only the first time options were processed (i.e., from the command line but not from attributes). It's a hack in that ideally we'd save the options structures before passing them through finish_options, options from attributes would apply to the saved structures and then all the finishing (including bits that aren't currently in finish_options) would be rerun - hopefully fixing some of the bugs relating to target and optimize attributes.
> On Fri, 7 Jan 2011, Jan Hubicka wrote: > > > Hmm, I didn't do that since finish_options seems to contain guard against > > repeated execution > > if (!opts->x_flag_opts_finished) > > { > > if (opts->x_flag_pie) > > opts->x_flag_pic = opts->x_flag_pie; > > if (opts->x_flag_pic && !opts->x_flag_pie) > > opts->x_flag_shlib = 1; > > opts->x_flag_opts_finished = false; > > } > > > only now I noticed that opts_finished is set to false that it is > > already. This looks like typo. > > That's definitely a mistake. > > > What was original motivation for that thing? > > x_flag_opts_finished is meant to be a replacement (avoiding global state) > for code that ran only the first time options were processed (i.e., from > the command line but not from attributes). It's a hack in that ideally > we'd save the options structures before passing them through > finish_options, options from attributes would apply to the saved > structures and then all the finishing (including bits that aren't > currently in finish_options) would be rerun - hopefully fixing some of the > bugs relating to target and optimize attributes. OK, thanks for explanation. Could you please care fixing this, or shall I drop it into TODO? Honza
On Fri, 7 Jan 2011, Jan Hubicka wrote: > > x_flag_opts_finished is meant to be a replacement (avoiding global state) > > for code that ran only the first time options were processed (i.e., from > > the command line but not from attributes). It's a hack in that ideally > > we'd save the options structures before passing them through > > finish_options, options from attributes would apply to the saved > > structures and then all the finishing (including bits that aren't > > currently in finish_options) would be rerun - hopefully fixing some of the > > bugs relating to target and optimize attributes. > > OK, thanks for explanation. Could you please care fixing this, or shall I drop > it into TODO? I'll deal with fixing the incorrect "false" setting.
Index: lto-opts.c =================================================================== --- lto-opts.c (revision 168506) +++ lto-opts.c (working copy) @@ -420,5 +420,9 @@ gcc_unreachable (); } + /* Flag_shlib is usually set by finish_options, but we are issing flag_pic + too late. */ + if (flag_pic && !flag_pie) + flag_shlib = 1; VEC_free (opt_t, heap, opts); }