| Message ID | 587F1F3A.6050208@samsung.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 18 Jan 2017, Maxim Ostapenko wrote: > Hi, > > as was figured out in PR LTO + ASan raises false initialization order fiasco > alarm due to in LTO case main_input_filename doesn't match module name passed > to __asan_before_dynamic_init. > Following Jakub's suggestion I used TRANSLATION_UNIT_DECL for corresponding > globals to overcome this issue (I needed to create a source location for each > TRANSLATION_UNIT_DECL). > However, when testing, I hit on a nasty issue: for some reason > linemap_ordinary_map_lookup, called from lto_output_location for given > TRANSLATION_UNIT_DECL, hit an assert: > > [...] > linemap_assert (line >= MAP_START_LOCATION (result)); > return result; > } > > due to line == 2. > > After some investigation I noticed that source locations are propagated > through location cache that can be partially invalidated by > lto_location_cache::revert_location_cache call. And that was my case: after > adding source location for TRANSLATION_UNIT_DECL into location cache, it was > reverted by calling lto_location_cache::revert_location_cache from unify_scc > before it was accepted: > > static void > lto_read_decls (struct lto_file_decl_data *decl_data, const void *data, > vec<ld_plugin_symbol_resolution_t> resolutions) > { > [...] > /* Try to unify the SCC with already existing ones. */ > if (!flag_ltrans > && unify_scc (data_in, from, > len, scc_entry_len, scc_hash)) > continue; > > For now I can overcome it by calling location_cache.accept_location_cache for > TRANSLATION_UNIT_DECL, but I wonder if more reliable fix is possible. > > Attached patch fixes the issue mentioned in PR and passes regression testing > and LTO bootstrap on x86_64-unknown-linux-gnu. > Could you please take a look on it? Looks good to me (aka, OK). Thanks, Richard. > -Maxim >
On 18/01/17 16:00, Richard Biener wrote: > On Wed, 18 Jan 2017, Maxim Ostapenko wrote: > >> Hi, >> >> as was figured out in PR LTO + ASan raises false initialization order fiasco >> alarm due to in LTO case main_input_filename doesn't match module name passed >> to __asan_before_dynamic_init. >> Following Jakub's suggestion I used TRANSLATION_UNIT_DECL for corresponding >> globals to overcome this issue (I needed to create a source location for each >> TRANSLATION_UNIT_DECL). >> However, when testing, I hit on a nasty issue: for some reason >> linemap_ordinary_map_lookup, called from lto_output_location for given >> TRANSLATION_UNIT_DECL, hit an assert: >> >> [...] >> linemap_assert (line >= MAP_START_LOCATION (result)); >> return result; >> } >> >> due to line == 2. >> >> After some investigation I noticed that source locations are propagated >> through location cache that can be partially invalidated by >> lto_location_cache::revert_location_cache call. And that was my case: after >> adding source location for TRANSLATION_UNIT_DECL into location cache, it was >> reverted by calling lto_location_cache::revert_location_cache from unify_scc >> before it was accepted: >> >> static void >> lto_read_decls (struct lto_file_decl_data *decl_data, const void *data, >> vec<ld_plugin_symbol_resolution_t> resolutions) >> { >> [...] >> /* Try to unify the SCC with already existing ones. */ >> if (!flag_ltrans >> && unify_scc (data_in, from, >> len, scc_entry_len, scc_hash)) >> continue; >> >> For now I can overcome it by calling location_cache.accept_location_cache for >> TRANSLATION_UNIT_DECL, but I wonder if more reliable fix is possible. >> >> Attached patch fixes the issue mentioned in PR and passes regression testing >> and LTO bootstrap on x86_64-unknown-linux-gnu. >> Could you please take a look on it? > Looks good to me (aka, OK). Thanks, applied as r244581. Is it OK to commit the patch on branches if no new errors pop up in the next few days? -Maxim > > Thanks, > Richard. > >> -Maxim >>
On January 18, 2017 5:17:12 PM GMT+01:00, Maxim Ostapenko <m.ostapenko@samsung.com> wrote: >On 18/01/17 16:00, Richard Biener wrote: >> On Wed, 18 Jan 2017, Maxim Ostapenko wrote: >> >>> Hi, >>> >>> as was figured out in PR LTO + ASan raises false initialization >order fiasco >>> alarm due to in LTO case main_input_filename doesn't match module >name passed >>> to __asan_before_dynamic_init. >>> Following Jakub's suggestion I used TRANSLATION_UNIT_DECL for >corresponding >>> globals to overcome this issue (I needed to create a source location >for each >>> TRANSLATION_UNIT_DECL). >>> However, when testing, I hit on a nasty issue: for some reason >>> linemap_ordinary_map_lookup, called from lto_output_location for >given >>> TRANSLATION_UNIT_DECL, hit an assert: >>> >>> [...] >>> linemap_assert (line >= MAP_START_LOCATION (result)); >>> return result; >>> } >>> >>> due to line == 2. >>> >>> After some investigation I noticed that source locations are >propagated >>> through location cache that can be partially invalidated by >>> lto_location_cache::revert_location_cache call. And that was my >case: after >>> adding source location for TRANSLATION_UNIT_DECL into location >cache, it was >>> reverted by calling lto_location_cache::revert_location_cache from >unify_scc >>> before it was accepted: >>> >>> static void >>> lto_read_decls (struct lto_file_decl_data *decl_data, const void >*data, >>> vec<ld_plugin_symbol_resolution_t> resolutions) >>> { >>> [...] >>> /* Try to unify the SCC with already existing ones. */ >>> if (!flag_ltrans >>> && unify_scc (data_in, from, >>> len, scc_entry_len, scc_hash)) >>> continue; >>> >>> For now I can overcome it by calling >location_cache.accept_location_cache for >>> TRANSLATION_UNIT_DECL, but I wonder if more reliable fix is >possible. >>> >>> Attached patch fixes the issue mentioned in PR and passes regression >testing >>> and LTO bootstrap on x86_64-unknown-linux-gnu. >>> Could you please take a look on it? >> Looks good to me (aka, OK). > >Thanks, applied as r244581. Is it OK to commit the patch on branches if > >no new errors pop up in the next few days? Yes. Richard. > >-Maxim > >> >> Thanks, >> Richard. >> >>> -Maxim >>>
On Wed, 18 Jan 2017, Maxim Ostapenko wrote: > On 18/01/17 16:00, Richard Biener wrote: > > On Wed, 18 Jan 2017, Maxim Ostapenko wrote: > > > > > Hi, > > > > > > as was figured out in PR LTO + ASan raises false initialization order > > > fiasco > > > alarm due to in LTO case main_input_filename doesn't match module name > > > passed > > > to __asan_before_dynamic_init. > > > Following Jakub's suggestion I used TRANSLATION_UNIT_DECL for > > > corresponding > > > globals to overcome this issue (I needed to create a source location for > > > each > > > TRANSLATION_UNIT_DECL). > > > However, when testing, I hit on a nasty issue: for some reason > > > linemap_ordinary_map_lookup, called from lto_output_location for given > > > TRANSLATION_UNIT_DECL, hit an assert: > > > > > > [...] > > > linemap_assert (line >= MAP_START_LOCATION (result)); > > > return result; > > > } > > > > > > due to line == 2. > > > > > > After some investigation I noticed that source locations are propagated > > > through location cache that can be partially invalidated by > > > lto_location_cache::revert_location_cache call. And that was my case: > > > after > > > adding source location for TRANSLATION_UNIT_DECL into location cache, it > > > was > > > reverted by calling lto_location_cache::revert_location_cache from > > > unify_scc > > > before it was accepted: > > > > > > static void > > > lto_read_decls (struct lto_file_decl_data *decl_data, const void *data, > > > vec<ld_plugin_symbol_resolution_t> resolutions) > > > { > > > [...] > > > /* Try to unify the SCC with already existing ones. */ > > > if (!flag_ltrans > > > && unify_scc (data_in, from, > > > len, scc_entry_len, scc_hash)) > > > continue; > > > > > > For now I can overcome it by calling location_cache.accept_location_cache > > > for > > > TRANSLATION_UNIT_DECL, but I wonder if more reliable fix is possible. > > > > > > Attached patch fixes the issue mentioned in PR and passes regression > > > testing > > > and LTO bootstrap on x86_64-unknown-linux-gnu. > > > Could you please take a look on it? > > Looks good to me (aka, OK). > > Thanks, applied as r244581. Is it OK to commit the patch on branches if no new > errors pop up in the next few days? So this regresses compile-time by 100% on some fortran cases. Doing the linemap operation in build_translation_unit_decl confuses the linemap code too much it seems. Please revert this (and not backport it) for now. A better place to store the filename would be eventually the DECL_NAME of it or a new field alongside TRANSLATION_UNIT_DECL_LANGUAGE. Thanks, Richard. > -Maxim > > > > > Thanks, > > Richard. > > > > > -Maxim > > > > >
On 23/01/17 12:03, Richard Biener wrote: > On Wed, 18 Jan 2017, Maxim Ostapenko wrote: > >> On 18/01/17 16:00, Richard Biener wrote: >>> On Wed, 18 Jan 2017, Maxim Ostapenko wrote: >>> >>>> Hi, >>>> >>>> as was figured out in PR LTO + ASan raises false initialization order >>>> fiasco >>>> alarm due to in LTO case main_input_filename doesn't match module name >>>> passed >>>> to __asan_before_dynamic_init. >>>> Following Jakub's suggestion I used TRANSLATION_UNIT_DECL for >>>> corresponding >>>> globals to overcome this issue (I needed to create a source location for >>>> each >>>> TRANSLATION_UNIT_DECL). >>>> However, when testing, I hit on a nasty issue: for some reason >>>> linemap_ordinary_map_lookup, called from lto_output_location for given >>>> TRANSLATION_UNIT_DECL, hit an assert: >>>> >>>> [...] >>>> linemap_assert (line >= MAP_START_LOCATION (result)); >>>> return result; >>>> } >>>> >>>> due to line == 2. >>>> >>>> After some investigation I noticed that source locations are propagated >>>> through location cache that can be partially invalidated by >>>> lto_location_cache::revert_location_cache call. And that was my case: >>>> after >>>> adding source location for TRANSLATION_UNIT_DECL into location cache, it >>>> was >>>> reverted by calling lto_location_cache::revert_location_cache from >>>> unify_scc >>>> before it was accepted: >>>> >>>> static void >>>> lto_read_decls (struct lto_file_decl_data *decl_data, const void *data, >>>> vec<ld_plugin_symbol_resolution_t> resolutions) >>>> { >>>> [...] >>>> /* Try to unify the SCC with already existing ones. */ >>>> if (!flag_ltrans >>>> && unify_scc (data_in, from, >>>> len, scc_entry_len, scc_hash)) >>>> continue; >>>> >>>> For now I can overcome it by calling location_cache.accept_location_cache >>>> for >>>> TRANSLATION_UNIT_DECL, but I wonder if more reliable fix is possible. >>>> >>>> Attached patch fixes the issue mentioned in PR and passes regression >>>> testing >>>> and LTO bootstrap on x86_64-unknown-linux-gnu. >>>> Could you please take a look on it? >>> Looks good to me (aka, OK). >> Thanks, applied as r244581. Is it OK to commit the patch on branches if no new >> errors pop up in the next few days? > So this regresses compile-time by 100% on some fortran cases. Doing the > linemap operation in build_translation_unit_decl confuses the linemap > code too much it seems. > > Please revert this (and not backport it) for now. Done with r244773, sorry about that :-( . We'll probably need to reopen the bug, right? -Maxim > > A better place to store the filename would be eventually the DECL_NAME > of it or a new field alongside TRANSLATION_UNIT_DECL_LANGUAGE. > > Thanks, > Richard. > >> -Maxim >> >>> Thanks, >>> Richard. >>> >>>> -Maxim >>>> >>
On Mon, Jan 23, 2017 at 12:14:21PM +0300, Maxim Ostapenko wrote: > > Please revert this (and not backport it) for now. > > Done with r244773, sorry about that :-( . We'll probably need to reopen the > bug, right? Sure. Jakub
gcc/lto/ChangeLog: 2017-01-17 Maxim Ostapenko <m.ostapenko@samsung.com> * lto.c (lto_read_decls): accept location cache for TRANSLATION_UNIT_DECL. gcc/testsuite/ChangeLog: 2017-01-17 Maxim Ostapenko <m.ostapenko@samsung.com> * gcc.dg/cpp/mi1.c: Adjust testcase. * gcc.dg/pch/cpp-3.c: Likewise. gcc/ChangeLog: 2017-01-17 Maxim Ostapenko <m.ostapenko@samsung.com> * asan.c (get_translation_unit_decl): New function. (asan_add_global): Extract modules file name from globals TRANSLATION_UNIT_DECL in lto mode. * tree.c (build_translation_unit_decl): Add source location for newly built TRANSLATION_UNIT_DECL. diff --git a/gcc/asan.c b/gcc/asan.c index 7450044..9a59fe4 100644 --- a/gcc/asan.c +++ b/gcc/asan.c @@ -2372,6 +2372,22 @@ asan_needs_odr_indicator_p (tree decl) && TREE_PUBLIC (decl)); } +/* For given DECL return its corresponding TRANSLATION_UNIT_DECL. */ + +static const_tree +get_translation_unit_decl (tree decl) +{ + const_tree context = decl; + while (context && TREE_CODE (context) != TRANSLATION_UNIT_DECL) + { + if (TREE_CODE (context) == BLOCK) + context = BLOCK_SUPERCONTEXT (context); + else + context = get_containing_scope (context); + } + return context; +} + /* Append description of a single global DECL into vector V. TYPE is __asan_global struct type as returned by asan_global_struct. */ @@ -2391,7 +2407,14 @@ asan_add_global (tree decl, tree type, vec<constructor_elt, va_gc> *v) pp_string (&asan_pp, "<unknown>"); str_cst = asan_pp_string (&asan_pp); - pp_string (&module_name_pp, main_input_filename); + const char *filename = main_input_filename; + if (in_lto_p) + { + const_tree translation_unit_decl = get_translation_unit_decl (decl); + if (translation_unit_decl) + filename = DECL_SOURCE_FILE (translation_unit_decl); + } + pp_string (&module_name_pp, filename); module_name_cst = asan_pp_string (&module_name_pp); if (asan_needs_local_alias (decl)) diff --git a/gcc/lto/lto.c b/gcc/lto/lto.c index d77d85d..c65e7cd 100644 --- a/gcc/lto/lto.c +++ b/gcc/lto/lto.c @@ -1707,7 +1707,13 @@ lto_read_decls (struct lto_file_decl_data *decl_data, const void *data, && (TREE_CODE (first) == IDENTIFIER_NODE || TREE_CODE (first) == INTEGER_CST || TREE_CODE (first) == TRANSLATION_UNIT_DECL)) - continue; + { + /* For TRANSLATION_UNIT_DECL we need to accept location cache now + to avoid possible reverting during following unify_scc call. */ + if (TREE_CODE (first) == TRANSLATION_UNIT_DECL) + data_in->location_cache.accept_location_cache (); + continue; + } /* Try to unify the SCC with already existing ones. */ if (!flag_ltrans diff --git a/gcc/testsuite/gcc.dg/cpp/mi1.c b/gcc/testsuite/gcc.dg/cpp/mi1.c index 0cfedad..9817431 100644 --- a/gcc/testsuite/gcc.dg/cpp/mi1.c +++ b/gcc/testsuite/gcc.dg/cpp/mi1.c @@ -13,7 +13,7 @@ /* { dg-do compile } { dg-options "-H" } - { dg-message "mi1c\.h\n\[^\n\]*mi1cc\.h\n\[^\n\]*mi1nd\.h\n\[^\n\]*mi1ndp\.h\n\[^\n\]*mi1x\.h" "redundant include check" { target *-*-* } 0 } */ + { dg-message "mi1c\.h\n\[^\n\]*mi1cc\.h\n\[^\n\]*mi1nd\.h\n\[^\n\]*mi1ndp\.h\n\[^\n\]*mi1x\.h\n\[^\n\]*mi1\.c" "redundant include check" { target *-*-* } 0 } */ #include "mi1c.h" #include "mi1c.h" diff --git a/gcc/testsuite/gcc.dg/pch/cpp-3.c b/gcc/testsuite/gcc.dg/pch/cpp-3.c index 25b5ca4..d635706 100644 --- a/gcc/testsuite/gcc.dg/pch/cpp-3.c +++ b/gcc/testsuite/gcc.dg/pch/cpp-3.c @@ -1,7 +1,7 @@ /* PR preprocessor/36649 */ /* { dg-do compile } */ /* { dg-options "-H -I." } */ -/* { dg-message "cpp-3.h\[^\n\]*(\n\[^\n\]*cpp-3.c)?\n\[^\n\]*cpp-3a.h\n\[^\n\]*cpp-3b.h" "" { target *-*-* } 0 } */ +/* { dg-message "cpp-3.h\[^\n\]*(\n\[^\n\]*cpp-3.c)?\n\[^\n\]*cpp-3a.h\n\[^\n\]*cpp-3b.h\n\[^\n\]*cpp-3.c" "" { target *-*-* } 0 } */ #include "cpp-3.h" #include "cpp-3a.h" diff --git a/gcc/tree.c b/gcc/tree.c index cffa36d..59fe8d4 100644 --- a/gcc/tree.c +++ b/gcc/tree.c @@ -4758,7 +4758,10 @@ vec<tree, va_gc> *all_translation_units; tree build_translation_unit_decl (tree name) { - tree tu = build_decl (UNKNOWN_LOCATION, TRANSLATION_UNIT_DECL, + linemap_add (line_table, LC_ENTER, false, main_input_filename, 1); + location_t loc = linemap_line_start (line_table, 1, 0); + linemap_add (line_table, LC_LEAVE, false, NULL, 0); + tree tu = build_decl (loc, TRANSLATION_UNIT_DECL, name, NULL_TREE); TRANSLATION_UNIT_LANGUAGE (tu) = lang_hooks.name; vec_safe_push (all_translation_units, tu);
Hi, as was figured out in PR LTO + ASan raises false initialization order fiasco alarm due to in LTO case main_input_filename doesn't match module name passed to __asan_before_dynamic_init. Following Jakub's suggestion I used TRANSLATION_UNIT_DECL for corresponding globals to overcome this issue (I needed to create a source location for each TRANSLATION_UNIT_DECL). However, when testing, I hit on a nasty issue: for some reason linemap_ordinary_map_lookup, called from lto_output_location for given TRANSLATION_UNIT_DECL, hit an assert: [...] linemap_assert (line >= MAP_START_LOCATION (result)); return result; } due to line == 2. After some investigation I noticed that source locations are propagated through location cache that can be partially invalidated by lto_location_cache::revert_location_cache call. And that was my case: after adding source location for TRANSLATION_UNIT_DECL into location cache, it was reverted by calling lto_location_cache::revert_location_cache from unify_scc before it was accepted: static void lto_read_decls (struct lto_file_decl_data *decl_data, const void *data, vec<ld_plugin_symbol_resolution_t> resolutions) { [...] /* Try to unify the SCC with already existing ones. */ if (!flag_ltrans && unify_scc (data_in, from, len, scc_entry_len, scc_hash)) continue; For now I can overcome it by calling location_cache.accept_location_cache for TRANSLATION_UNIT_DECL, but I wonder if more reliable fix is possible. Attached patch fixes the issue mentioned in PR and passes regression testing and LTO bootstrap on x86_64-unknown-linux-gnu. Could you please take a look on it? -Maxim