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
