Message ID | ead1fa8a-41ac-45c8-9cdc-192fdae28082@gmail.com |
---|---|
State | New |
Headers | show |
Series | avoid error_mark_node in -Wsizeof-pointer-memaccess (PR 88065) | expand |
On 11/17/18 3:45 PM, Martin Sebor wrote: > -Wsizeof-pointer-memaccess fails with an ICE when one of > the arguments is ill-formed (error_mark_node). To avoid > the error the attached patch has the function bail in this > case. > > Martin > > gcc-88065.diff > > PR c/88065 - ICE in -Wsizeof-pointer-memaccess on an invalid strncpy > > gcc/c-family/ChangeLog: > > PR c/88065 > * c-warn.c (sizeof_pointer_memaccess_warning): Bail if source > or destination is an error. > > gcc/testsuite/ChangeLog: > > PR c/88065 > * gcc.dg/Wsizeof-pointer-memaccess2.c: New test. This is probably OK. But before final ACK, is there a point earlier where we could/should have bailed out? ie, when does the ERROR_MARK get created and if you were to look at the flow from that point to the offending call to sizeof_pointer_memaccess_warning is there a better place to bail? jeff
On Mon, Nov 19, 2018 at 04:10:09PM -0700, Jeff Law wrote: > > PR c/88065 - ICE in -Wsizeof-pointer-memaccess on an invalid strncpy > > > > gcc/c-family/ChangeLog: > > > > PR c/88065 > > * c-warn.c (sizeof_pointer_memaccess_warning): Bail if source > > or destination is an error. > > > > gcc/testsuite/ChangeLog: > > > > PR c/88065 > > * gcc.dg/Wsizeof-pointer-memaccess2.c: New test. > This is probably OK. But before final ACK, is there a point earlier > where we could/should have bailed out? IMHO it is a good point, but it should use error_operand_p predicate instead of == error_mark_node checks to also catch the case where the argument is not error_mark_node, but has error_mark_node type. And, the testcase shouldn't be in gcc.dg, but in c-c++-common and cover also C++ testing. Jakub
On 11/19/2018 04:10 PM, Jeff Law wrote: > On 11/17/18 3:45 PM, Martin Sebor wrote: >> -Wsizeof-pointer-memaccess fails with an ICE when one of >> the arguments is ill-formed (error_mark_node). To avoid >> the error the attached patch has the function bail in this >> case. >> >> Martin >> >> gcc-88065.diff >> >> PR c/88065 - ICE in -Wsizeof-pointer-memaccess on an invalid strncpy >> >> gcc/c-family/ChangeLog: >> >> PR c/88065 >> * c-warn.c (sizeof_pointer_memaccess_warning): Bail if source >> or destination is an error. >> >> gcc/testsuite/ChangeLog: >> >> PR c/88065 >> * gcc.dg/Wsizeof-pointer-memaccess2.c: New test. > This is probably OK. But before final ACK, is there a point earlier > where we could/should have bailed out? > > ie, when does the ERROR_MARK get created and if you were to look at the > flow from that point to the offending call to > sizeof_pointer_memaccess_warning is there a better place to bail? sizeof_pointer_memaccess_warning() has a big switch statement with the built-in code as a controlling expression that it uses to determine which argument is the source, which one is the destination, and which one is the size/bound. To do the checking anywhere else up the stack the caller would have to replicate the same switch statement, so I think this is the right spot to do it. I can move the test if it's important. FWIW, I tend to only add rests to c-c++-common if they exercise different code between the two front-ends. Otherwise it seems like unnecessarily increasing the already excessive testsuite runtimes (each of the C++ tests runs once for each C++ revision, i.e., C++ 98, C++ 11, C++ 14, and C++ 17). I do realize that if the implementation were to diverge between the two front-ends having test coverage for both would be a good thing. So it's a trade-off. While griping about c-c++-common: sometimes (though not in the case of ICEs), because of subtle differences between the two front-ends (like missing or inaccurate location information) it can also be a pain to get the tests to produce the same output even for a shared implementation of a warning, let alone for distinct ones. Martin
On 11/19/18 5:36 PM, Martin Sebor wrote: > On 11/19/2018 04:10 PM, Jeff Law wrote: >> On 11/17/18 3:45 PM, Martin Sebor wrote: >>> -Wsizeof-pointer-memaccess fails with an ICE when one of >>> the arguments is ill-formed (error_mark_node). To avoid >>> the error the attached patch has the function bail in this >>> case. >>> >>> Martin >>> >>> gcc-88065.diff >>> >>> PR c/88065 - ICE in -Wsizeof-pointer-memaccess on an invalid strncpy >>> >>> gcc/c-family/ChangeLog: >>> >>> PR c/88065 >>> * c-warn.c (sizeof_pointer_memaccess_warning): Bail if source >>> or destination is an error. >>> >>> gcc/testsuite/ChangeLog: >>> >>> PR c/88065 >>> * gcc.dg/Wsizeof-pointer-memaccess2.c: New test. >> This is probably OK. But before final ACK, is there a point earlier >> where we could/should have bailed out? >> >> ie, when does the ERROR_MARK get created and if you were to look at the >> flow from that point to the offending call to >> sizeof_pointer_memaccess_warning is there a better place to bail? > > sizeof_pointer_memaccess_warning() has a big switch statement > with the built-in code as a controlling expression that it > uses to determine which argument is the source, which one is > the destination, and which one is the size/bound. To do > the checking anywhere else up the stack the caller would have > to replicate the same switch statement, so I think this is > the right spot to do it. > > I can move the test if it's important. I don't think it's terribly important. It's just part of the "did we get here in a reasonable way" kind of question I always try to ask myself with these kinds of patches. > > FWIW, I tend to only add rests to c-c++-common if they exercise > different code between the two front-ends. Otherwise it seems > like unnecessarily increasing the already excessive testsuite > runtimes (each of the C++ tests runs once for each C++ revision, > i.e., C++ 98, C++ 11, C++ 14, and C++ 17). I do realize that > if the implementation were to diverge between the two front-ends > having test coverage for both would be a good thing. So it's > a trade-off. While griping about c-c++-common: sometimes (though > not in the case of ICEs), because of subtle differences between > the two front-ends (like missing or inaccurate location > information) it can also be a pain to get the tests to produce > the same output even for a shared implementation of a warning, > let alone for distinct ones. I don't have strong opinions on this issue with this patch, so I'll defer to Jakub on this issue if the test can be easily moved. So moving the test and using error_operand_p rather than testing for ERROR_MARK_NODE and this is OK for the trunk. jeff
On Tue, Nov 20, 2018 at 12:39:44AM +0100, Jakub Jelinek wrote: > On Mon, Nov 19, 2018 at 04:10:09PM -0700, Jeff Law wrote: > > > PR c/88065 - ICE in -Wsizeof-pointer-memaccess on an invalid strncpy > > > > > > gcc/c-family/ChangeLog: > > > > > > PR c/88065 Please also add PR c/87297 > > > * c-warn.c (sizeof_pointer_memaccess_warning): Bail if source > > > or destination is an error. > > > > > > gcc/testsuite/ChangeLog: > > > > > > PR c/88065 > > > * gcc.dg/Wsizeof-pointer-memaccess2.c: New test. > > This is probably OK. But before final ACK, is there a point earlier > > where we could/should have bailed out? > > IMHO it is a good point, but it should use error_operand_p predicate instead > of == error_mark_node checks to also catch the case where the argument is > not error_mark_node, but has error_mark_node type. And, the testcase > shouldn't be in gcc.dg, but in c-c++-common and cover also C++ testing. Testcase proving that error_operand_p is really necessary: /* PR c/87297 */ /* { dg-do compile } */ /* { dg-options "-Wsizeof-pointer-memaccess" } */ struct S { char a[4]; }; void foo (struct S *p, const char *s) { struct T x; /* { dg-error "storage size|incomplete type" } */ __builtin_strncpy (x, s, sizeof p->a); } Works in C, still ICEs in C++ even with the patch you've posted. 819 tree dstsz = TYPE_SIZE_UNIT (TREE_TYPE (d)); debug_tree (d) <var_decl 0x7ffff7ff6c60 x type <error_mark 0x7fffefc7fdf8> used decl_5 VOID huvaa.c:9:12 align:8 warn_if_not_align:0 context <function_decl 0x7fffefdda200 foo> chain <type_decl 0x7fffefda1850 T>> And, I think it is important to have these tests in c-c++-common, as the above test shows, it behaves differently between C and C++ (C will present error_mark_node itself rather than VAR_DECL with error_mark_node type) and the code in question is just a helper for the FEs. Jakub
On 11/21/18 3:04 AM, Jakub Jelinek wrote: > On Tue, Nov 20, 2018 at 12:39:44AM +0100, Jakub Jelinek wrote: >> On Mon, Nov 19, 2018 at 04:10:09PM -0700, Jeff Law wrote: >>>> PR c/88065 - ICE in -Wsizeof-pointer-memaccess on an invalid strncpy >>>> >>>> gcc/c-family/ChangeLog: >>>> >>>> PR c/88065 > > Please also add > PR c/87297 Done. > >>>> * c-warn.c (sizeof_pointer_memaccess_warning): Bail if source >>>> or destination is an error. >>>> >>>> gcc/testsuite/ChangeLog: >>>> >>>> PR c/88065 >>>> * gcc.dg/Wsizeof-pointer-memaccess2.c: New test. >>> This is probably OK. But before final ACK, is there a point earlier >>> where we could/should have bailed out? >> >> IMHO it is a good point, but it should use error_operand_p predicate instead >> of == error_mark_node checks to also catch the case where the argument is >> not error_mark_node, but has error_mark_node type. And, the testcase >> shouldn't be in gcc.dg, but in c-c++-common and cover also C++ testing. > > Testcase proving that error_operand_p is really necessary: > > /* PR c/87297 */ > /* { dg-do compile } */ > /* { dg-options "-Wsizeof-pointer-memaccess" } */ > struct S { char a[4]; }; > > void > foo (struct S *p, const char *s) > { > struct T x; /* { dg-error "storage size|incomplete type" } */ > __builtin_strncpy (x, s, sizeof p->a); > } > > Works in C, still ICEs in C++ even with the patch you've posted. Thanks for the test case! I didn't know about error_operand_p or that this could happen. I wonder how many other bugs there are lurking in there because of it. > 819 tree dstsz = TYPE_SIZE_UNIT (TREE_TYPE (d)); > debug_tree (d) > <var_decl 0x7ffff7ff6c60 x type <error_mark 0x7fffefc7fdf8> > used decl_5 VOID huvaa.c:9:12 > align:8 warn_if_not_align:0 context <function_decl 0x7fffefdda200 foo> > chain <type_decl 0x7fffefda1850 T>> > > And, I think it is important to have these tests in c-c++-common, as the > above test shows, it behaves differently between C and C++ (C will present > error_mark_node itself rather than VAR_DECL with error_mark_node type) and > the code in question is just a helper for the FEs. Given how easy it is to trip over the error type I have to agree. I made the change to error_operand_p, moved the test and committed r266594. Martin
PR c/88065 - ICE in -Wsizeof-pointer-memaccess on an invalid strncpy gcc/c-family/ChangeLog: PR c/88065 * c-warn.c (sizeof_pointer_memaccess_warning): Bail if source or destination is an error. gcc/testsuite/ChangeLog: PR c/88065 * gcc.dg/Wsizeof-pointer-memaccess2.c: New test. Index: gcc/c-family/c-warn.c =================================================================== --- gcc/c-family/c-warn.c (revision 266240) +++ gcc/c-family/c-warn.c (working copy) @@ -784,7 +784,10 @@ sizeof_pointer_memaccess_warning (location_t *size if (idx >= 3) return; - if (sizeof_arg[idx] == NULL || sizeof_arg[idx] == error_mark_node) + if (src == error_mark_node + || dest == error_mark_node + || sizeof_arg[idx] == NULL + || sizeof_arg[idx] == error_mark_node) return; type = TYPE_P (sizeof_arg[idx]) Index: gcc/testsuite/gcc.dg/Wsizeof-pointer-memaccess2.c =================================================================== --- gcc/testsuite/gcc.dg/Wsizeof-pointer-memaccess2.c (nonexistent) +++ gcc/testsuite/gcc.dg/Wsizeof-pointer-memaccess2.c (working copy) @@ -0,0 +1,24 @@ +/* PR c/88065 - ICE in -Wsizeof-pointer-memaccess on an invalid strncpy + { dg-do compile } + { dg-options "-Wall" } */ + +typedef __SIZE_TYPE__ size_t; + +char* strncpy (char*, const char*, size_t); + +struct S { char a[4], b[6]; }; + +void test_invalid_dst (struct S *p) +{ + strncpy (q->a, p->b, sizeof p->b); /* { dg-error ".q. undeclared" } */ +} + +void test_invalid_src (struct S *p) +{ + strncpy (p->a, q->b, sizeof p->b); /* { dg-error ".q. undeclared" } */ +} + +void test_invalid_bound (struct S *p) +{ + strncpy (p->a, p->b, sizeof q->b); /* { dg-error ".q. undeclared" } */ +}