| Message ID | 543D51CC.6050202@samsung.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Oct 14, 2014 at 08:39:40PM +0400, Ilya Palachev wrote: > Attached patch fixes PR lto/61048 - > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61048 Given that the patch just replaces an ICE with a necessary link failure, I'd say it is done at the wrong place, instead during the LTO option handling you should error out if there are incompatibilities in -fsanitize options (any object compiled with flag_sanitize & SANITIZE_USER_ADDRESS, but link done without that, ditto for SANITIZE_KERNEL_ADDRESS, SANITIZE_THREAD. And finally if flag_sanitize & (SANITIZE_UNDEFINED | SANITIZE_NONDEFALT) is non-zero but during linking it is zero (it doesn't really matter which exact undefined sanitization options are used at what time). BTW, in your patches please watch formatting, you didn't use space before (. Jakub
On Wed, Oct 15, 2014 at 8:59 AM, Jakub Jelinek <jakub@redhat.com> wrote: > On Tue, Oct 14, 2014 at 08:39:40PM +0400, Ilya Palachev wrote: >> Attached patch fixes PR lto/61048 - >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61048 > > Given that the patch just replaces an ICE with a necessary link failure, I'd say > it is done at the wrong place, instead during the LTO option handling you > should error out if there are incompatibilities in -fsanitize options > (any object compiled with flag_sanitize & SANITIZE_USER_ADDRESS, but > link done without that, ditto for SANITIZE_KERNEL_ADDRESS, SANITIZE_THREAD. > And finally if flag_sanitize & (SANITIZE_UNDEFINED | SANITIZE_NONDEFALT) > is non-zero but during linking it is zero (it doesn't really matter which > exact undefined sanitization options are used at what time). Yep. As with other options this looks like it needs "conservative" merging. Which might be more involved than for other cases - but well... Look at existing examples in gcc/lto-opts.c and gcc/lto-wrapper.c. Richard. > BTW, in your patches please watch formatting, you didn't use space before (. > > Jakub
On 15.10.2014 10:59, Jakub Jelinek wrote: > On Tue, Oct 14, 2014 at 08:39:40PM +0400, Ilya Palachev wrote: >> Attached patch fixes PR lto/61048 - >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61048 > > Given that the patch just replaces an ICE with a necessary link failure, I'd say > it is done at the wrong place, instead during the LTO option handling you > should error out if there are incompatibilities in -fsanitize options > (any object compiled with flag_sanitize & SANITIZE_USER_ADDRESS, but > link done without that, ditto for SANITIZE_KERNEL_ADDRESS, SANITIZE_THREAD. > And finally if flag_sanitize & (SANITIZE_UNDEFINED | SANITIZE_NONDEFALT) > is non-zero but during linking it is zero (it doesn't really matter which > exact undefined sanitization options are used at what time). As I understand you suggest that the compiler should: - Write compiler options with which the object file was compiled to the ELF format in a special section. - Read these sections during the linking command - Compare collected sets of options with options that were set at linking command. ? Does compiler have any opportunity to save the set of options with which the object file was compiled inside some special ELF section? For LTO they the special section ".gnu.lto_.opts" is used. But not all compiler options are written to it: $ gcc -fsanitize=address -c test.cpp -o test.o -flto $ objdump --full-contents --section=.gnu.lto_.opts test.o test.o: file format elf64-x86-64 Contents of section .gnu.lto_.opts: 0000 272d6665 78636570 74696f6e 73272027 '-fexceptions' ' 0010 2d666d61 74682d65 72726e6f 2720272d -fmath-errno' '- 0020 66736967 6e65642d 7a65726f 73272027 fsigned-zeros' ' 0030 2d667472 61707069 6e672d6d 61746827 -ftrapping-math' 0040 20272d66 6e6f2d74 72617076 2720272d '-fno-trapv' '- 0050 666e6f2d 73747269 63742d6f 76657266 fno-strict-overf 0060 6c6f7727 20272d6d 74756e65 3d67656e low' '-mtune=gen 0070 65726963 2720272d 6d617263 683d7838 eric' '-march=x8 0080 362d3634 2720272d 666c746f 2700 6-64' '-flto'. As you can see there is no "-fsanitize" option here. Only options that are handled in function lto_write_options in lto-opts.c are written there. Does gcc has an opportunity to write something like ELF section ".gnu.opts" to save all compiler options with which the object file was compiled? Or we need to introduce such section? Or anything else? > > BTW, in your patches please watch formatting, you didn't use space before (. Sorry for that. I'll check it in further patches. > > Jakub >
On 15.10.2014 12:09, Richard Biener wrote: > On Wed, Oct 15, 2014 at 8:59 AM, Jakub Jelinek <jakub@redhat.com> wrote: >> On Tue, Oct 14, 2014 at 08:39:40PM +0400, Ilya Palachev wrote: >>> Attached patch fixes PR lto/61048 - >>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61048 >> >> Given that the patch just replaces an ICE with a necessary link failure, I'd say >> it is done at the wrong place, instead during the LTO option handling you >> should error out if there are incompatibilities in -fsanitize options >> (any object compiled with flag_sanitize & SANITIZE_USER_ADDRESS, but >> link done without that, ditto for SANITIZE_KERNEL_ADDRESS, SANITIZE_THREAD. >> And finally if flag_sanitize & (SANITIZE_UNDEFINED | SANITIZE_NONDEFALT) >> is non-zero but during linking it is zero (it doesn't really matter which >> exact undefined sanitization options are used at what time). > > Yep. As with other options this looks like it needs "conservative" > merging. Which might be more involved than for other cases - but well... > > Look at existing examples in gcc/lto-opts.c and gcc/lto-wrapper.c. I have seen that in r180827 you have removed functions lto_read_file_options from gcc/lto-streamer.h and lto_read_all_file_options from gcc/lto/lto.c. Do you mean that something like these functions should be added again? Maybe it will be worthwhile to report the user what options he forget to add to the linkning command? For example, without -flto option the following error is reported: $ g++ -c test.cpp -fsanitize=address -o test_nolto.o $ g++ test_nolto.o -o test_nolto test_nolto.o:test.cpp:function main: error: undefined reference to '__asan_report_load4' test_nolto.o:test.cpp:function __static_initialization_and_destruction_0(int, int): error: undefined reference to '__asan_before_dynamic_init' test_nolto.o:test.cpp:function __static_initialization_and_destruction_0(int, int): error: undefined reference to '__asan_after_dynamic_init' test_nolto.o:test.cpp:function _GLOBAL__sub_D_00099_0_main: error: undefined reference to '__asan_unregister_globals' test_nolto.o:test.cpp:function _GLOBAL__sub_I_00099_1_main: error: undefined reference to '__asan_init_v4' test_nolto.o:test.cpp:function _GLOBAL__sub_I_00099_1_main: error: undefined reference to '__asan_register_globals' collect2: error: ld returned 1 exit status Maybe it will be better to print something like: test_nolto.o:test.cpp: error: compiled with -fsanitize=address, but this option is not specified in linking command. ? Thanks, Ilya > > Richard. > >> BTW, in your patches please watch formatting, you didn't use space before (. >> >> Jakub >
On Wed, Oct 15, 2014 at 10:18 AM, Ilya Palachev <i.palachev@samsung.com> wrote: > On 15.10.2014 10:59, Jakub Jelinek wrote: >> >> On Tue, Oct 14, 2014 at 08:39:40PM +0400, Ilya Palachev wrote: >>> >>> Attached patch fixes PR lto/61048 - >>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61048 >> >> >> Given that the patch just replaces an ICE with a necessary link failure, >> I'd say >> it is done at the wrong place, instead during the LTO option handling you >> should error out if there are incompatibilities in -fsanitize options >> (any object compiled with flag_sanitize & SANITIZE_USER_ADDRESS, but >> link done without that, ditto for SANITIZE_KERNEL_ADDRESS, >> SANITIZE_THREAD. >> And finally if flag_sanitize & (SANITIZE_UNDEFINED | SANITIZE_NONDEFALT) >> is non-zero but during linking it is zero (it doesn't really matter which >> exact undefined sanitization options are used at what time). > > > As I understand you suggest that the compiler should: > - Write compiler options with which the object file was compiled to the ELF > format in a special section. > - Read these sections during the linking command > - Compare collected sets of options with options that were set at linking > command. > ? > > Does compiler have any opportunity to save the set of options with which the > object file was compiled inside some special ELF section? For LTO they the > special section ".gnu.lto_.opts" is used. But not all compiler options are > written to it: > > $ gcc -fsanitize=address -c test.cpp -o test.o -flto > $ objdump --full-contents --section=.gnu.lto_.opts test.o > > test.o: file format elf64-x86-64 > > Contents of section .gnu.lto_.opts: > 0000 272d6665 78636570 74696f6e 73272027 '-fexceptions' ' > 0010 2d666d61 74682d65 72726e6f 2720272d -fmath-errno' '- > 0020 66736967 6e65642d 7a65726f 73272027 fsigned-zeros' ' > 0030 2d667472 61707069 6e672d6d 61746827 -ftrapping-math' > 0040 20272d66 6e6f2d74 72617076 2720272d '-fno-trapv' '- > 0050 666e6f2d 73747269 63742d6f 76657266 fno-strict-overf > 0060 6c6f7727 20272d6d 74756e65 3d67656e low' '-mtune=gen > 0070 65726963 2720272d 6d617263 683d7838 eric' '-march=x8 > 0080 362d3634 2720272d 666c746f 2700 6-64' '-flto'. > > As you can see there is no "-fsanitize" option here. Only options that are > handled in function lto_write_options in lto-opts.c are written there. > > Does gcc has an opportunity to write something like ELF section ".gnu.opts" > to save all compiler options with which the object file was compiled? > Or we need to introduce such section? Or anything else? You need to handle them in lto-opts.c and output them to the existing option section. Not sure why they don't appear there already - ah, because of /* Also drop all options that are handled by the driver as well, which includes things like -o and -v or -fhelp for example. We do not need those. Also drop all diagnostic options. */ if (cl_options[option->opt_index].flags & (CL_DRIVER|CL_WARNING)) continue; and -fsanitize= being marked as Driver. Looks like we have to drop things explicitely here given that -o is also Common Driver. Or simply have if (cl_options[option->opt_index].flags & (CL_DRIVER|CL_WARNING)) switch (option->opt_index) { case OPT_fsanitize_: break; default: continue; } instead. Richard. >> >> BTW, in your patches please watch formatting, you didn't use space before >> (. > > > Sorry for that. I'll check it in further patches. > >> >> Jakub >> >
On Wed, Oct 15, 2014 at 12:41:47PM +0400, Ilya Palachev wrote: > >Yep. As with other options this looks like it needs "conservative" > >merging. Which might be more involved than for other cases - but well... > > > >Look at existing examples in gcc/lto-opts.c and gcc/lto-wrapper.c. > > I have seen that in r180827 you have removed functions lto_read_file_options > from gcc/lto-streamer.h and lto_read_all_file_options from gcc/lto/lto.c. Do > you mean that something like these functions should be added again? I don't understand why would you want to read/write all options, just make sure -fsanitize= is among those that are important for LTO. I'll defer to Richard/Honza where exactly in LTO that should be done. > Maybe it will be worthwhile to report the user what options he forget to add > to the linkning command? > > For example, without -flto option the following error is reported: > > $ g++ -c test.cpp -fsanitize=address -o test_nolto.o > $ g++ test_nolto.o -o test_nolto > test_nolto.o:test.cpp:function main: error: undefined reference to > '__asan_report_load4' For non-lto, there shouldn't be any merging of options from anywhere. gcc man page documents that the -fsanitize= options are needed for linking too, it is similar to -fopenmp or plenty of other options. Nothing special needs to be done for that. Jakub
From 926a8b84a52f3120c3f71cd28e0d782c719b7791 Mon Sep 17 00:00:00 2001 From: Ilya Palachev <i.palachev@samsung.com> Date: Tue, 14 Oct 2014 19:22:32 +0400 Subject: [PATCH] Define missed builtins on demand gcc/ 2014-10-14 Ilya Palachev <i.palachev@samsung.com> * langhooks.h (define_builtin_on_demand): New function. * langhooks-def.h (LANG_HOOKS_DEFINE_BUILTIN_ON_DEMAND): New macro. * lto/lto-lang.c (lto_define_builtin_on_demand): New function. * tree-streamer-in.c (streamer_get_builtin_tree): Use define_builtin_on_demand in case when the declaration of builtin is missing. gcc/testsuite/ 2014-10-14 Ilya Palachev <i.palachev@samsung.com> * g++.dg/lto/pr61048_0.C: New test from bugzilla. --- gcc/langhooks-def.h | 4 +++- gcc/langhooks.h | 3 +++ gcc/lto/lto-lang.c | 16 ++++++++++++++++ gcc/testsuite/g++.dg/lto/pr61048_0.C | 10 ++++++++++ gcc/tree-streamer-in.c | 4 ++++ 5 files changed, 36 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/g++.dg/lto/pr61048_0.C diff --git a/gcc/langhooks-def.h b/gcc/langhooks-def.h index e5ae3e3..2ddccbc 100644 --- a/gcc/langhooks-def.h +++ b/gcc/langhooks-def.h @@ -254,11 +254,13 @@ extern void lhd_end_section (void); #define LANG_HOOKS_BEGIN_SECTION lhd_begin_section #define LANG_HOOKS_APPEND_DATA lhd_append_data #define LANG_HOOKS_END_SECTION lhd_end_section +#define LANG_HOOKS_DEFINE_BUILTIN_ON_DEMAND 0 #define LANG_HOOKS_LTO { \ LANG_HOOKS_BEGIN_SECTION, \ LANG_HOOKS_APPEND_DATA, \ - LANG_HOOKS_END_SECTION \ + LANG_HOOKS_END_SECTION, \ + LANG_HOOKS_DEFINE_BUILTIN_ON_DEMAND \ } /* The whole thing. The structure is defined in langhooks.h. */ diff --git a/gcc/langhooks.h b/gcc/langhooks.h index 32e76f9..a0cbe5f 100644 --- a/gcc/langhooks.h +++ b/gcc/langhooks.h @@ -255,6 +255,9 @@ struct lang_hooks_for_lto /* End the previously begun LTO section. */ void (*end_section) (void); + + /* Define builtin on demand. */ + void (*define_builtin_on_demand) (enum built_in_function fcode); }; /* Language-specific hooks. See langhooks-def.h for defaults. */ diff --git a/gcc/lto/lto-lang.c b/gcc/lto/lto-lang.c index 9e8524a..4d458b2 100644 --- a/gcc/lto/lto-lang.c +++ b/gcc/lto/lto-lang.c @@ -697,6 +697,19 @@ lto_define_builtins (tree va_list_ref_type_node ATTRIBUTE_UNUSED, #undef DEF_BUILTIN } +static void +lto_define_builtin_on_demand(enum built_in_function fcode) +{ +#define DEF_BUILTIN(ENUM, NAME, CLASS, TYPE, LIBTYPE, BOTH_P, FALLBACK_P,\ + NONANSI_P, ATTRS, IMPLICIT, COND) \ + if (NAME && (ENUM == fcode)) \ + def_builtin_1 (ENUM, NAME, CLASS, builtin_types[(int) TYPE], \ + builtin_types[(int) LIBTYPE], BOTH_P, FALLBACK_P, \ + NONANSI_P, built_in_attributes[(int) ATTRS], IMPLICIT); +#include "builtins.def" +#undef DEF_BUILTIN +} + static GTY(()) tree registered_builtin_types; /* Language hooks. */ @@ -1308,6 +1321,9 @@ static void lto_init_ts (void) #undef LANG_HOOKS_INIT_TS #define LANG_HOOKS_INIT_TS lto_init_ts +#undef LANG_HOOKS_DEFINE_BUILTIN_ON_DEMAND +#define LANG_HOOKS_DEFINE_BUILTIN_ON_DEMAND lto_define_builtin_on_demand + struct lang_hooks lang_hooks = LANG_HOOKS_INITIALIZER; /* Language hooks that are not part of lang_hooks. */ diff --git a/gcc/testsuite/g++.dg/lto/pr61048_0.C b/gcc/testsuite/g++.dg/lto/pr61048_0.C new file mode 100644 index 0000000..e5f4961 --- /dev/null +++ b/gcc/testsuite/g++.dg/lto/pr61048_0.C @@ -0,0 +1,10 @@ +// { dg-lto-do link } +// { dg-lto-options {{-fsanitize=address -flto}} } + +#include <iostream> +int main() +{ + int *i = reinterpret_cast<int*>(0xC1000000); + std::cout << *i << std::endl; +} + diff --git a/gcc/tree-streamer-in.c b/gcc/tree-streamer-in.c index 01a55bf..6387f33 100644 --- a/gcc/tree-streamer-in.c +++ b/gcc/tree-streamer-in.c @@ -36,6 +36,7 @@ along with GCC; see the file COPYING3. If not see #include "streamer-hooks.h" #include "lto-streamer.h" #include "builtins.h" +#include "langhooks.h" /* Read a STRING_CST from the string table in DATA_IN using input block IB. */ @@ -1115,6 +1116,9 @@ streamer_get_builtin_tree (struct lto_input_block *ib, struct data_in *data_in) if (fcode >= END_BUILTINS) fatal_error ("machine independent builtin code out of range"); result = builtin_decl_explicit (fcode); + if (!result) + lang_hooks.lto.define_builtin_on_demand(fcode); + result = builtin_decl_explicit (fcode); gcc_assert (result); } else if (fclass == BUILT_IN_MD) -- 2.1.1
Hi all, Attached patch fixes PR lto/61048 - https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61048 The reason of failure was that the builtin information structure was not initialized properly at the link stage. The failed assertion was caused by missing builtin declaration ( BUILT_IN_ASAN_AFTER_DYNAMIC_INIT), which was requested from this structure. As usual this information should be initialized in function lto_define_builtins, which is called from LTO lang hook function lto_init. But in the given testcase the initialization did not happen, since the declaration is initialized only if the following condition holds: (flag_sanitize & (SANITIZE_ADDRESS | SANITIZE_THREAD \ | SANITIZE_UNDEFINED | SANITIZE_NONDEFAULT)) But if the user compiles (without linking) file in LTO mode with -fsanitize=address option, and then tries to link the executable from *.o file, but does not specify option -fsanitize=address, variable flag_sanitize will be 0 and sanitizer builtins info will not be initialized, and ICE will happen. Commands to reproduce the problem: g++ test.cpp -c -o test.o -fsanitize=address -flto g++ test.o -o test -Wl,-flto # At this stage flag_sanitize is 0, and sanitizer builtins are not defined. The simplest way to fix this seems to add initialization of sanitizer builtins using function initialize_sanitizer_builtins - and this helps to avoid ICE: jobserver. */ @@ -1856,6 +1857,9 @@ lto_read_decls (struct lto_file_decl_data *decl_data, const void *data, data_in = lto_data_in_create (decl_data, (const char *) data + string_offset, header->string_size, resolutions); + /* Initialize sanitizer builtins if necessary. */ + initialize_sanitizer_builtins(); + /* We do not uniquify the pre-loaded cache entries, those are middle-end internal types that should not be merged. */ ---- But this approach means that asan-specific functions must be called from lto. The suggested patch proposes another approach: add definitions of builtins during the final stage, when they are requested from builtin_info structure. I have tried to do it by adding lto-specific lang-hook, so that to reuse existing code for builtins initialization (currently builtins are initialized in lto_init hook). In the attached patch such hook is added, and it is used in streamer_get_builtin_tree. It seems that the discussed issue can happen not only for flag -fsanitize, but also for all options that cause the definition of builtins, so the proposed patch is independent from sanitizers. The patch was bootstrapped and regtested on x86_64-unknown-linux-gnu. Ok for trunk? Best regards, Ilya Palachev