| Message ID | c05119df-8679-d3b4-3d13-31a870ac18cc@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
On Tue, Feb 23, 2021 at 10:42 AM Martin Liška <mliska@suse.cz> wrote: > > Hello. > > The patch is about confusion that brings ICF when it merged 2 variables > with different alignments (when ASAN is used). > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > > Ready to be installed? Can't we fix the asan runtime? Does the same issue happen when merging two comdat with different alignment and LTO? > Thanks, > Martin > > gcc/ChangeLog: > > PR sanitizer/99168 > * ipa-icf.c (sem_variable::merge): Do not merge 2 variables > with different alignment. That leads to an invalid red zone > size allocated in runtime. > > gcc/testsuite/ChangeLog: > > PR sanitizer/99168 > * c-c++-common/asan/pr99168.c: New test. > --- > gcc/ipa-icf.c | 13 ++++++++++++ > gcc/testsuite/c-c++-common/asan/pr99168.c | 26 +++++++++++++++++++++++ > 2 files changed, 39 insertions(+) > create mode 100644 gcc/testsuite/c-c++-common/asan/pr99168.c > > diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c > index 687ad8d45b7..5dd33a75c3a 100644 > --- a/gcc/ipa-icf.c > +++ b/gcc/ipa-icf.c > @@ -88,6 +88,7 @@ along with GCC; see the file COPYING3. If not see > #include "tree-vector-builder.h" > #include "symtab-thunks.h" > #include "alias.h" > +#include "asan.h" > > using namespace ipa_icf_gimple; > > @@ -2022,6 +2023,18 @@ sem_variable::merge (sem_item *alias_item) > return false; > } > > + if (DECL_ALIGN (original->decl) != DECL_ALIGN (alias->decl) > + && (sanitize_flags_p (SANITIZE_ADDRESS, original->decl) > + || sanitize_flags_p (SANITIZE_ADDRESS, alias->decl))) > + { > + if (dump_enabled_p ()) > + dump_printf (MSG_MISSED_OPTIMIZATION, > + "Not unifying; " > + "ASAN requires equal alignments for original and alias\n"); > + > + return false; > + } > + > if (DECL_ALIGN (original->decl) < DECL_ALIGN (alias->decl)) > { > if (dump_enabled_p ()) > diff --git a/gcc/testsuite/c-c++-common/asan/pr99168.c b/gcc/testsuite/c-c++-common/asan/pr99168.c > new file mode 100644 > index 00000000000..ed59ffb3d48 > --- /dev/null > +++ b/gcc/testsuite/c-c++-common/asan/pr99168.c > @@ -0,0 +1,26 @@ > +/* PR sanitizer/99168 */ > +/* { dg-do run } */ > + > +struct my_struct > +{ > + unsigned long volatile x; > +} __attribute__((aligned(128))); > + > +static int variablek[5][6] = {}; > +static struct my_struct variables1 = {0UL}; > +static struct my_struct variables2 __attribute__((aligned(32))) = {0UL}; > + > +int main() { > + int i, j; > + for (i = 0; i < 5; i++) { > + for (j = 0; j < 6; j++) { > + __builtin_printf("%d ", variablek[i][j]); > + } > + } > + __builtin_printf("\n"); > + > + __builtin_printf("%lu\n", variables1.x); > + __builtin_printf("%lu\n", variables2.x); > + > + return 0; > +} > -- > 2.30.1 >
On 2/23/21 12:56 PM, Richard Biener wrote: > Can't we fix the asan runtime? Does the same issue happen when merging > two comdat with different alignment and LTO? All right, there's a detail explanation what happens. Let's consider the following example: struct my_struct { unsigned long volatile x; } __attribute__((aligned(128))); static int array[5][6] = {}; static struct my_struct variable128 = {1UL}; static struct my_struct variable32 __attribute__((aligned(64))) = {1UL}; Here we have 2 variables (variable128 and variable32) that are merged. Later on, we decide not to protect the global variable variable128 due to: || DECL_ALIGN_UNIT (decl) > 2 * ASAN_RED_ZONE_SIZE Without ICF we end up with: .align 64 .type variable32, @object .size variable32, 128 variable32: .zero 128 .zero 32 .align 128 .type variable128, @object .size variable128, 128 variable128: .zero 128 As seen, variable32 has .zero 128+32, where 32 is the red-zone (and alignment is increased to 64). With ICF we end up with: .align 128 .type variable128, @object .size variable128, 128 variable128: .zero 128 .set variable32,variable128 So variable32 points to variable128 (which has no prepared red zone + alignment is the same). $ nm -n a.out ... 0000000000400b80 r variable128 0000000000400b80 r variable32 0000000000400c00 r array 0000000000400c00 - 0000000000400b80 == sizeof(variable32). Then we tell libasan what is the variable size and size of the corresponding red zone: $ ASAN_OPTIONS=report_globals=3 ./a.out ... ==20602==Added Global[0x000000403080]: beg=0x000000400b80 size=128/160 name=variable32 module=asan.c dyn_init=0 odr_indicator=0x000000000000 And bad thinks happen. So I really think ICF should not merge the variables. Please provide a comdat test-case :) Thanks, Martin
On Tue, Feb 23, 2021 at 3:22 PM Martin Liška <mliska@suse.cz> wrote: > > On 2/23/21 12:56 PM, Richard Biener wrote: > > Can't we fix the asan runtime? Does the same issue happen when merging > > two comdat with different alignment and LTO? > > All right, there's a detail explanation what happens. > Let's consider the following example: > > struct my_struct > { > unsigned long volatile x; > } __attribute__((aligned(128))); > > static int array[5][6] = {}; > static struct my_struct variable128 = {1UL}; > static struct my_struct variable32 __attribute__((aligned(64))) = {1UL}; > > Here we have 2 variables (variable128 and variable32) that are merged. Later on, > we decide not to protect the global variable variable128 due to: > || DECL_ALIGN_UNIT (decl) > 2 * ASAN_RED_ZONE_SIZE > > Without ICF we end up with: > > .align 64 > .type variable32, @object > .size variable32, 128 > variable32: > .zero 128 > .zero 32 > .align 128 > .type variable128, @object > .size variable128, 128 > variable128: > .zero 128 > > As seen, variable32 has .zero 128+32, where 32 is the red-zone (and alignment is increased to 64). > > With ICF we end up with: > > .align 128 > .type variable128, @object > .size variable128, 128 > variable128: > .zero 128 > .set variable32,variable128 > > So variable32 points to variable128 (which has no prepared red zone + alignment is the same). > $ nm -n a.out > ... > 0000000000400b80 r variable128 > 0000000000400b80 r variable32 > 0000000000400c00 r array > > 0000000000400c00 - 0000000000400b80 == sizeof(variable32). > > Then we tell libasan what is the variable size and size of the corresponding red zone: > $ ASAN_OPTIONS=report_globals=3 ./a.out > ... > ==20602==Added Global[0x000000403080]: beg=0x000000400b80 size=128/160 name=variable32 module=asan.c dyn_init=0 odr_indicator=0x000000000000 Ah, so the issue is that ASAN still sees both variables (and isn't properly cgraph/varpool aware)? So instead of just keying on different alignment you'd have to verify in ICF whether the decls are "registered the same" by ASAN, no? Or simply not perform any variable ICF when ASAN is enabled? > And bad thinks happen. So I really think ICF should not merge the variables. > Please provide a comdat test-case :) > > Thanks, > Martin > >
On 2/23/21 3:32 PM, Richard Biener wrote: > On Tue, Feb 23, 2021 at 3:22 PM Martin Liška <mliska@suse.cz> wrote: >> >> On 2/23/21 12:56 PM, Richard Biener wrote: >>> Can't we fix the asan runtime? Does the same issue happen when merging >>> two comdat with different alignment and LTO? >> >> All right, there's a detail explanation what happens. >> Let's consider the following example: >> >> struct my_struct >> { >> unsigned long volatile x; >> } __attribute__((aligned(128))); >> >> static int array[5][6] = {}; >> static struct my_struct variable128 = {1UL}; >> static struct my_struct variable32 __attribute__((aligned(64))) = {1UL}; >> >> Here we have 2 variables (variable128 and variable32) that are merged. Later on, >> we decide not to protect the global variable variable128 due to: >> || DECL_ALIGN_UNIT (decl) > 2 * ASAN_RED_ZONE_SIZE >> >> Without ICF we end up with: >> >> .align 64 >> .type variable32, @object >> .size variable32, 128 >> variable32: >> .zero 128 >> .zero 32 >> .align 128 >> .type variable128, @object >> .size variable128, 128 >> variable128: >> .zero 128 >> >> As seen, variable32 has .zero 128+32, where 32 is the red-zone (and alignment is increased to 64). >> >> With ICF we end up with: >> >> .align 128 >> .type variable128, @object >> .size variable128, 128 >> variable128: >> .zero 128 >> .set variable32,variable128 >> >> So variable32 points to variable128 (which has no prepared red zone + alignment is the same). >> $ nm -n a.out >> ... >> 0000000000400b80 r variable128 >> 0000000000400b80 r variable32 >> 0000000000400c00 r array >> >> 0000000000400c00 - 0000000000400b80 == sizeof(variable32). >> >> Then we tell libasan what is the variable size and size of the corresponding red zone: >> $ ASAN_OPTIONS=report_globals=3 ./a.out >> ... >> ==20602==Added Global[0x000000403080]: beg=0x000000400b80 size=128/160 name=variable32 module=asan.c dyn_init=0 odr_indicator=0x000000000000 > > Ah, so the issue is that ASAN still sees both variables (and isn't > properly cgraph/varpool aware)? No, in both cases the variable128 is not handled by ASAN (it has too big alignment). > So instead of just > keying on different alignment you'd have to verify in ICF whether the > decls are "registered the same" by ASAN, no? Yes, ICF is too optimistic about alignment of global variables. I'm not sure I want to call asan_protect_global from ICF. > Or simply not perform any variable ICF when ASAN is enabled? I think the suggested patch should tell ICF to be strict about alignment when ASAN is enabled. Note the issue is quite hairy :) Martin > >> And bad thinks happen. So I really think ICF should not merge the variables. >> Please provide a comdat test-case :) >> >> Thanks, >> Martin >> >>
On Tue, Feb 23, 2021 at 3:41 PM Martin Liška <mliska@suse.cz> wrote: > > On 2/23/21 3:32 PM, Richard Biener wrote: > > On Tue, Feb 23, 2021 at 3:22 PM Martin Liška <mliska@suse.cz> wrote: > >> > >> On 2/23/21 12:56 PM, Richard Biener wrote: > >>> Can't we fix the asan runtime? Does the same issue happen when merging > >>> two comdat with different alignment and LTO? > >> > >> All right, there's a detail explanation what happens. > >> Let's consider the following example: > >> > >> struct my_struct > >> { > >> unsigned long volatile x; > >> } __attribute__((aligned(128))); > >> > >> static int array[5][6] = {}; > >> static struct my_struct variable128 = {1UL}; > >> static struct my_struct variable32 __attribute__((aligned(64))) = {1UL}; > >> > >> Here we have 2 variables (variable128 and variable32) that are merged. Later on, > >> we decide not to protect the global variable variable128 due to: > >> || DECL_ALIGN_UNIT (decl) > 2 * ASAN_RED_ZONE_SIZE > >> > >> Without ICF we end up with: > >> > >> .align 64 > >> .type variable32, @object > >> .size variable32, 128 > >> variable32: > >> .zero 128 > >> .zero 32 > >> .align 128 > >> .type variable128, @object > >> .size variable128, 128 > >> variable128: > >> .zero 128 > >> > >> As seen, variable32 has .zero 128+32, where 32 is the red-zone (and alignment is increased to 64). > >> > >> With ICF we end up with: > >> > >> .align 128 > >> .type variable128, @object > >> .size variable128, 128 > >> variable128: > >> .zero 128 > >> .set variable32,variable128 > >> > >> So variable32 points to variable128 (which has no prepared red zone + alignment is the same). > >> $ nm -n a.out > >> ... > >> 0000000000400b80 r variable128 > >> 0000000000400b80 r variable32 > >> 0000000000400c00 r array > >> > >> 0000000000400c00 - 0000000000400b80 == sizeof(variable32). > >> > >> Then we tell libasan what is the variable size and size of the corresponding red zone: > >> $ ASAN_OPTIONS=report_globals=3 ./a.out > >> ... > >> ==20602==Added Global[0x000000403080]: beg=0x000000400b80 size=128/160 name=variable32 module=asan.c dyn_init=0 odr_indicator=0x000000000000 > > > > Ah, so the issue is that ASAN still sees both variables (and isn't > > properly cgraph/varpool aware)? > > No, in both cases the variable128 is not handled by ASAN (it has too big alignment). > > > So instead of just > > keying on different alignment you'd have to verify in ICF whether the > > decls are "registered the same" by ASAN, no? > > Yes, ICF is too optimistic about alignment of global variables. I'm not sure > I want to call asan_protect_global from ICF. > > > Or simply not perform any variable ICF when ASAN is enabled? > > I think the suggested patch should tell ICF to be strict about alignment > when ASAN is enabled. Sure. The question is whether there's more issues with ASAN on-the-side tracking of stuff. > Note the issue is quite hairy :) Understood, I guess the patch is OK but it doesn't look very nice to sprinkle such checks around the code that might "confuse" ASAN. For example there's the vectorizer "IPA" pass that increases alignment of globals. I know nothing of ASAN but it sounds like it produces its tables too early. Richard. > Martin > > > > >> And bad thinks happen. So I really think ICF should not merge the variables. > >> Please provide a comdat test-case :) > >> > >> Thanks, > >> Martin > >> > >> >
On 2/23/21 3:55 PM, Richard Biener wrote: > On Tue, Feb 23, 2021 at 3:41 PM Martin Liška <mliska@suse.cz> wrote: >> >> On 2/23/21 3:32 PM, Richard Biener wrote: >>> On Tue, Feb 23, 2021 at 3:22 PM Martin Liška <mliska@suse.cz> wrote: >>>> >>>> On 2/23/21 12:56 PM, Richard Biener wrote: >>>>> Can't we fix the asan runtime? Does the same issue happen when merging >>>>> two comdat with different alignment and LTO? >>>> >>>> All right, there's a detail explanation what happens. >>>> Let's consider the following example: >>>> >>>> struct my_struct >>>> { >>>> unsigned long volatile x; >>>> } __attribute__((aligned(128))); >>>> >>>> static int array[5][6] = {}; >>>> static struct my_struct variable128 = {1UL}; >>>> static struct my_struct variable32 __attribute__((aligned(64))) = {1UL}; >>>> >>>> Here we have 2 variables (variable128 and variable32) that are merged. Later on, >>>> we decide not to protect the global variable variable128 due to: >>>> || DECL_ALIGN_UNIT (decl) > 2 * ASAN_RED_ZONE_SIZE >>>> >>>> Without ICF we end up with: >>>> >>>> .align 64 >>>> .type variable32, @object >>>> .size variable32, 128 >>>> variable32: >>>> .zero 128 >>>> .zero 32 >>>> .align 128 >>>> .type variable128, @object >>>> .size variable128, 128 >>>> variable128: >>>> .zero 128 >>>> >>>> As seen, variable32 has .zero 128+32, where 32 is the red-zone (and alignment is increased to 64). >>>> >>>> With ICF we end up with: >>>> >>>> .align 128 >>>> .type variable128, @object >>>> .size variable128, 128 >>>> variable128: >>>> .zero 128 >>>> .set variable32,variable128 >>>> >>>> So variable32 points to variable128 (which has no prepared red zone + alignment is the same). >>>> $ nm -n a.out >>>> ... >>>> 0000000000400b80 r variable128 >>>> 0000000000400b80 r variable32 >>>> 0000000000400c00 r array >>>> >>>> 0000000000400c00 - 0000000000400b80 == sizeof(variable32). >>>> >>>> Then we tell libasan what is the variable size and size of the corresponding red zone: >>>> $ ASAN_OPTIONS=report_globals=3 ./a.out >>>> ... >>>> ==20602==Added Global[0x000000403080]: beg=0x000000400b80 size=128/160 name=variable32 module=asan.c dyn_init=0 odr_indicator=0x000000000000 >>> >>> Ah, so the issue is that ASAN still sees both variables (and isn't >>> properly cgraph/varpool aware)? >> >> No, in both cases the variable128 is not handled by ASAN (it has too big alignment). >> >>> So instead of just >>> keying on different alignment you'd have to verify in ICF whether the >>> decls are "registered the same" by ASAN, no? >> >> Yes, ICF is too optimistic about alignment of global variables. I'm not sure >> I want to call asan_protect_global from ICF. >> >>> Or simply not perform any variable ICF when ASAN is enabled? >> >> I think the suggested patch should tell ICF to be strict about alignment >> when ASAN is enabled. > > Sure. The question is whether there's more issues with ASAN on-the-side > tracking of stuff. I hope not, ASAN is quite heavily tested. > >> Note the issue is quite hairy :) > > Understood, I guess the patch is OK but it doesn't look very nice to sprinkle > such checks around the code that might "confuse" ASAN. For example > there's the vectorizer "IPA" pass that increases alignment of globals. > I know nothing of ASAN but it sounds like it produces its tables too early. Well ASAN runs at the very end of TREE passes, so it should see all globals that are modified in the described way. Anyway, I've just installed the patch. Thanks, Martin > > Richard. > >> Martin >> >>> >>>> And bad thinks happen. So I really think ICF should not merge the variables. >>>> Please provide a comdat test-case :) >>>> >>>> Thanks, >>>> Martin >>>> >>>> >>
diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c index 687ad8d45b7..5dd33a75c3a 100644 --- a/gcc/ipa-icf.c +++ b/gcc/ipa-icf.c @@ -88,6 +88,7 @@ along with GCC; see the file COPYING3. If not see #include "tree-vector-builder.h" #include "symtab-thunks.h" #include "alias.h" +#include "asan.h" using namespace ipa_icf_gimple; @@ -2022,6 +2023,18 @@ sem_variable::merge (sem_item *alias_item) return false; } + if (DECL_ALIGN (original->decl) != DECL_ALIGN (alias->decl) + && (sanitize_flags_p (SANITIZE_ADDRESS, original->decl) + || sanitize_flags_p (SANITIZE_ADDRESS, alias->decl))) + { + if (dump_enabled_p ()) + dump_printf (MSG_MISSED_OPTIMIZATION, + "Not unifying; " + "ASAN requires equal alignments for original and alias\n"); + + return false; + } + if (DECL_ALIGN (original->decl) < DECL_ALIGN (alias->decl)) { if (dump_enabled_p ()) diff --git a/gcc/testsuite/c-c++-common/asan/pr99168.c b/gcc/testsuite/c-c++-common/asan/pr99168.c new file mode 100644 index 00000000000..ed59ffb3d48 --- /dev/null +++ b/gcc/testsuite/c-c++-common/asan/pr99168.c @@ -0,0 +1,26 @@ +/* PR sanitizer/99168 */ +/* { dg-do run } */ + +struct my_struct +{ + unsigned long volatile x; +} __attribute__((aligned(128))); + +static int variablek[5][6] = {}; +static struct my_struct variables1 = {0UL}; +static struct my_struct variables2 __attribute__((aligned(32))) = {0UL}; + +int main() { + int i, j; + for (i = 0; i < 5; i++) { + for (j = 0; j < 6; j++) { + __builtin_printf("%d ", variablek[i][j]); + } + } + __builtin_printf("\n"); + + __builtin_printf("%lu\n", variables1.x); + __builtin_printf("%lu\n", variables2.x); + + return 0; +}