diff mbox series

avoid error_mark_node in -Wsizeof-pointer-memaccess (PR 88065)

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

Commit Message

Martin Sebor Nov. 17, 2018, 10:45 p.m. UTC 
-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

Comments

Jeff Law Nov. 19, 2018, 11:10 p.m. UTC | #1 
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
Jakub Jelinek Nov. 19, 2018, 11:39 p.m. UTC | #2 
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
Martin Sebor Nov. 20, 2018, 12:36 a.m. UTC | #3 
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
Jeff Law Nov. 21, 2018, 12:41 a.m. UTC | #4 
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
Jakub Jelinek Nov. 21, 2018, 10:04 a.m. UTC | #5 
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
Martin Sebor Nov. 28, 2018, 11:13 p.m. UTC | #6 
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
diff mbox series

Patch

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" } */
+}