diff mbox series

[v3] diagnostics: Follow DECL_ORIGIN in lhd_print_error_function [PR102061]

Message ID 20240808025230.16913-1-peter0x44@disroot.org
State New
Headers show
Series [v3] diagnostics: Follow DECL_ORIGIN in lhd_print_error_function [PR102061] | expand

Commit Message

Peter0x44 Aug. 8, 2024, 2:52 a.m. UTC 
Currently, if a warning references a cloned function, the name of the cloned
function will be emitted in the "In function 'xyz'" part of the diagnostic,
which users aren't supposed to see. This patch follows the DECL_ORIGIN link
to get the name of the original function, so the internal compiler details
aren't exposed.

gcc/ChangeLog:
	PR diagnostics/102061
	* langhooks.cc (lhd_print_error_function): Follow DECL_ORIGIN
	links.
	* gcc.dg/pr102061.c: New testcase.

Signed-off-by: Peter Damianov <peter0x44@disroot.org>
---
v3: also follow DECL_ORIGIN when emitting "inlined from" warnings, I missed this before.
Add testcase.

 gcc/langhooks.cc                |  3 +++
 gcc/testsuite/gcc.dg/pr102061.c | 35 +++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)
 create mode 100644 gcc/testsuite/gcc.dg/pr102061.c

Comments

Richard Biener Aug. 8, 2024, 8:04 a.m. UTC | #1 
On Thu, Aug 8, 2024 at 4:55 AM Peter Damianov <peter0x44@disroot.org> wrote:
>
> Currently, if a warning references a cloned function, the name of the cloned
> function will be emitted in the "In function 'xyz'" part of the diagnostic,
> which users aren't supposed to see. This patch follows the DECL_ORIGIN link
> to get the name of the original function, so the internal compiler details
> aren't exposed.

Note I see an almost exact copy of the function in cp/error.cc as
cp_print_error_function (possibly more modern), specifically using

          pp_printf (context->printer, function_category (fndecl),
                     fndecl);

which ends up using %qD.

I've CCed David who likely invented diagnostic_abstract_origin and friends.

> gcc/ChangeLog:
>         PR diagnostics/102061
>         * langhooks.cc (lhd_print_error_function): Follow DECL_ORIGIN
>         links.
>         * gcc.dg/pr102061.c: New testcase.
>
> Signed-off-by: Peter Damianov <peter0x44@disroot.org>
> ---
> v3: also follow DECL_ORIGIN when emitting "inlined from" warnings, I missed this before.
> Add testcase.
>
>  gcc/langhooks.cc                |  3 +++
>  gcc/testsuite/gcc.dg/pr102061.c | 35 +++++++++++++++++++++++++++++++++
>  2 files changed, 38 insertions(+)
>  create mode 100644 gcc/testsuite/gcc.dg/pr102061.c
>
> diff --git a/gcc/langhooks.cc b/gcc/langhooks.cc
> index 61f2b676256..7a2a66b3c39 100644
> --- a/gcc/langhooks.cc
> +++ b/gcc/langhooks.cc
> @@ -395,6 +395,8 @@ lhd_print_error_function (diagnostic_context *context, const char *file,
>           else
>             fndecl = current_function_decl;
>
> +         fndecl = DECL_ORIGIN(fndecl);

Space after DECL_ORIGIN.  There's a comment warranted for what we
intend do to here.

I think this change is reasonable.

> +
>           if (TREE_CODE (TREE_TYPE (fndecl)) == METHOD_TYPE)
>             pp_printf
>               (context->printer, _("In member function %qs"),
> @@ -439,6 +441,7 @@ lhd_print_error_function (diagnostic_context *context, const char *file,
>                 }
>               if (fndecl)
>                 {
> +                 fndecl = DECL_ORIGIN(fndecl);

Space missing again.

This change OTOH might cause us to print

    inlined from foo at ...
    inlined from foo at ...

so duplicating an inline for example in the case we split a function and then
inline both parts or in the case we inline a IPA-CP forwarder and the specific
clone.  It's not obvious what we should do here since of course for a recursive
function we can have a function inlined two times in a row.

The testcase only triggers the first case, right?

David, any comments?  I think the patch is OK with the formatting fixed.

Thanks,
Richard.

>                   expanded_location s = expand_location (*locus);
>                   pp_comma (context->printer);
>                   pp_newline (context->printer);
> diff --git a/gcc/testsuite/gcc.dg/pr102061.c b/gcc/testsuite/gcc.dg/pr102061.c
> new file mode 100644
> index 00000000000..dbdd23965e7
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr102061.c
> @@ -0,0 +1,35 @@
> +/* { dg-do compile } */
> +/* { dg-options "-Wall -O2" } */
> +/* { dg-message "inlined from 'bar'" "" { target *-*-* } 0 } */
> +/* { dg-excess-errors "" } */
> +
> +static inline void
> +foo (char *p)
> +{
> +  __builtin___memcpy_chk (p, "abc", 3, __builtin_object_size (p, 0));
> +}
> +static void
> +bar (char *p) __attribute__((noinline));
> +static void
> +bar (char *p)
> +{
> +  foo (p);
> +}
> +void f(char*) __attribute__((noipa));
> +char buf[2];
> +void
> +baz (void) __attribute__((noinline));
> +void
> +baz (void)
> +{
> +  bar (buf);
> +  f(buf);
> +}
> +
> +void f(char*)
> +{}
> +
> +int main(void)
> +{
> +    baz();
> +}
> --
> 2.39.2
>
Peter0x44 Aug. 8, 2024, 11:54 a.m. UTC | #2 
On 2024-08-08 09:04, Richard Biener wrote:
> On Thu, Aug 8, 2024 at 4:55 AM Peter Damianov <peter0x44@disroot.org> 
> wrote:
>> 
>> Currently, if a warning references a cloned function, the name of the 
>> cloned
>> function will be emitted in the "In function 'xyz'" part of the 
>> diagnostic,
>> which users aren't supposed to see. This patch follows the DECL_ORIGIN 
>> link
>> to get the name of the original function, so the internal compiler 
>> details
>> aren't exposed.
> 
> Note I see an almost exact copy of the function in cp/error.cc as
> cp_print_error_function (possibly more modern), specifically using
I noticed that too, but I'm not sure what circumstances it is used 
under. I checked my patch removed the cloned names in the diagnostic for 
both C and C++.
> 
>           pp_printf (context->printer, function_category (fndecl),
>                      fndecl);
> 
> which ends up using %qD.
> 
> I've CCed David who likely invented diagnostic_abstract_origin and 
> friends.
> 
>> gcc/ChangeLog:
>>         PR diagnostics/102061
>>         * langhooks.cc (lhd_print_error_function): Follow DECL_ORIGIN
>>         links.
>>         * gcc.dg/pr102061.c: New testcase.
>> 
>> Signed-off-by: Peter Damianov <peter0x44@disroot.org>
>> ---
>> v3: also follow DECL_ORIGIN when emitting "inlined from" warnings, I 
>> missed this before.
>> Add testcase.
>> 
>>  gcc/langhooks.cc                |  3 +++
>>  gcc/testsuite/gcc.dg/pr102061.c | 35 
>> +++++++++++++++++++++++++++++++++
>>  2 files changed, 38 insertions(+)
>>  create mode 100644 gcc/testsuite/gcc.dg/pr102061.c
>> 
>> diff --git a/gcc/langhooks.cc b/gcc/langhooks.cc
>> index 61f2b676256..7a2a66b3c39 100644
>> --- a/gcc/langhooks.cc
>> +++ b/gcc/langhooks.cc
>> @@ -395,6 +395,8 @@ lhd_print_error_function (diagnostic_context 
>> *context, const char *file,
>>           else
>>             fndecl = current_function_decl;
>> 
>> +         fndecl = DECL_ORIGIN(fndecl);
> 
> Space after DECL_ORIGIN.  There's a comment warranted for what we
> intend do to here.
> 
> I think this change is reasonable.
> 
>> +
>>           if (TREE_CODE (TREE_TYPE (fndecl)) == METHOD_TYPE)
>>             pp_printf
>>               (context->printer, _("In member function %qs"),
>> @@ -439,6 +441,7 @@ lhd_print_error_function (diagnostic_context 
>> *context, const char *file,
>>                 }
>>               if (fndecl)
>>                 {
>> +                 fndecl = DECL_ORIGIN(fndecl);
> 
> Space missing again.
> 
> This change OTOH might cause us to print
> 
>     inlined from foo at ...
>     inlined from foo at ...
> 
> so duplicating an inline for example in the case we split a function 
> and then
> inline both parts or in the case we inline a IPA-CP forwarder and the 
> specific
> clone.  It's not obvious what we should do here since of course for a 
> recursive
> function we can have a function inlined two times in a row.
> 
> The testcase only triggers the first case, right?
Correct. I don't know how to construct a testcase exercising that, the 
thing that made me notice this first was actually a case with ".isra" 
and not ".constprop", but it would be nice for completeness. And also 
having a testcase that covers the other path, not just inlining.
> 
> David, any comments?  I think the patch is OK with the formatting 
> fixed.
> 
> Thanks,
> Richard.
> 
>>                   expanded_location s = expand_location (*locus);
>>                   pp_comma (context->printer);
>>                   pp_newline (context->printer);
>> diff --git a/gcc/testsuite/gcc.dg/pr102061.c 
>> b/gcc/testsuite/gcc.dg/pr102061.c
>> new file mode 100644
>> index 00000000000..dbdd23965e7
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/pr102061.c
>> @@ -0,0 +1,35 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-Wall -O2" } */
>> +/* { dg-message "inlined from 'bar'" "" { target *-*-* } 0 } */
>> +/* { dg-excess-errors "" } */
>> +
>> +static inline void
>> +foo (char *p)
>> +{
>> +  __builtin___memcpy_chk (p, "abc", 3, __builtin_object_size (p, 0));
>> +}
>> +static void
>> +bar (char *p) __attribute__((noinline));
>> +static void
>> +bar (char *p)
>> +{
>> +  foo (p);
>> +}
>> +void f(char*) __attribute__((noipa));
>> +char buf[2];
>> +void
>> +baz (void) __attribute__((noinline));
>> +void
>> +baz (void)
>> +{
>> +  bar (buf);
>> +  f(buf);
>> +}
>> +
>> +void f(char*)
>> +{}
>> +
>> +int main(void)
>> +{
>> +    baz();
>> +}
>> --
>> 2.39.2
>> 
Thanks for the review,
Peter D.
Peter0x44 Aug. 11, 2024, 3:53 a.m. UTC | #3 
> so duplicating an inline for example in the case we split a function 
> and then
> inline both parts or in the case we inline a IPA-CP forwarder and the 
> specific
> clone.  It's not obvious what we should do here since of course for a 
> recursive
> function we can have a function inlined two times in a row.

I think this is already happening. I'm not regressing anything.

> The testcase only triggers the first case, right?
In v4 I have changed part of the testcase so, I think does exercise this 
now:
https://gcc.godbolt.org/z/KdYPh39js

The testcase however does include infinite recursion. But it may still 
be relevant to real-world code.

static int bar(char* p) {
     __builtin_strncpy(p, p, 1);
     bar(p);
     return 0;
}

void baz() {
     char c[0];
     bar(c);
}

Outputs:
<source>: In function 'bar':
<source>:4:12: warning: infinite recursion detected 
[-Winfinite-recursion]
     4 | static int bar(char* p) {
       |            ^~~
<source>:6:5: note: recursive call
     6 |     bar(p);
       |     ^~~~~~
<source>: In function 'bar.isra':
<source>:5:5: warning: '__builtin_strncpy' source argument is the same 
as destination [-Wrestrict]
     5 |     __builtin_strncpy(p, p, 1);
       |     ^~~~~~~~~~~~~~~~~~~~~~~~~~
In function 'bar',
     inlined from 'bar.isra' at <source>:6:5:
<source>:5:5: warning: '__builtin_strncpy' source argument is the same 
as destination [-Wrestrict]
     5 |     __builtin_strncpy(p, p, 1);
       |     ^~~~~~~~~~~~~~~~~~~~~~~~~~
In function 'bar',
     inlined from 'bar' at <source>:6:5,
     inlined from 'bar.isra' at <source>:6:5:
<source>:5:5: warning: '__builtin_strncpy' source argument is the same 
as destination [-Wrestrict]
     5 |     __builtin_strncpy(p, p, 1);
       |     ^~~~~~~~~~~~~~~~~~~~~~~~~~
In function 'bar',
     inlined from 'bar' at <source>:6:5,
     inlined from 'bar' at <source>:6:5,
     inlined from 'bar.isra' at <source>:6:5:
<source>:5:5: warning: '__builtin_strncpy' source argument is the same 
as destination [-Wrestrict]
     5 |     __builtin_strncpy(p, p, 1);
       |     ^~~~~~~~~~~~~~~~~~~~~~~~~~
In function 'bar',
     inlined from 'bar' at <source>:6:5,
     inlined from 'bar' at <source>:6:5,
     inlined from 'bar' at <source>:6:5,
     inlined from 'bar.isra' at <source>:6:5:
<source>:5:5: warning: '__builtin_strncpy' source argument is the same 
as destination [-Wrestrict]
     5 |     __builtin_strncpy(p, p, 1);
       |     ^~~~~~~~~~~~~~~~~~~~~~~~~~
In function 'bar',
     inlined from 'bar' at <source>:6:5,
     inlined from 'bar' at <source>:6:5,
     inlined from 'bar' at <source>:6:5,
     inlined from 'bar' at <source>:6:5,
     inlined from 'bar.isra' at <source>:6:5:
<source>:5:5: warning: '__builtin_strncpy' source argument is the same 
as destination [-Wrestrict]
     5 |     __builtin_strncpy(p, p, 1);
       |     ^~~~~~~~~~~~~~~~~~~~~~~~~~
In function 'bar',
     inlined from 'bar' at <source>:6:5,
     inlined from 'bar' at <source>:6:5,
     inlined from 'bar' at <source>:6:5,
     inlined from 'bar' at <source>:6:5,
     inlined from 'bar' at <source>:6:5,
     inlined from 'bar.isra' at <source>:6:5:
<source>:5:5: warning: '__builtin_strncpy' source argument is the same 
as destination [-Wrestrict]
     5 |     __builtin_strncpy(p, p, 1);
       |     ^~~~~~~~~~~~~~~~~~~~~~~~~~
In function 'bar',
     inlined from 'bar' at <source>:6:5,
     inlined from 'bar' at <source>:6:5,
     inlined from 'bar' at <source>:6:5,
     inlined from 'bar' at <source>:6:5,
     inlined from 'bar' at <source>:6:5,
     inlined from 'bar' at <source>:6:5,
     inlined from 'bar.isra' at <source>:6:5:
<source>:5:5: warning: '__builtin_strncpy' source argument is the same 
as destination [-Wrestrict]
     5 |     __builtin_strncpy(p, p, 1);
       |     ^~~~~~~~~~~~~~~~~~~~~~~~~~
In function 'bar',
     inlined from 'bar' at <source>:6:5,
     inlined from 'bar' at <source>:6:5,
     inlined from 'bar' at <source>:6:5,
     inlined from 'bar' at <source>:6:5,
     inlined from 'bar' at <source>:6:5,
     inlined from 'bar' at <source>:6:5,
     inlined from 'bar' at <source>:6:5,
     inlined from 'bar.isra' at <source>:6:5:
<source>:5:5: warning: '__builtin_strncpy' source argument is the same 
as destination [-Wrestrict]
     5 |     __builtin_strncpy(p, p, 1);
       |     ^~~~~~~~~~~~~~~~~~~~~~~~~~
In function 'bar',
     inlined from 'baz' at <source>:12:5:
<source>:5:5: warning: '__builtin_strncpy' offset 0 is out of the bounds 
[0, 0] of object 'c' with type 'char[0]' [-Warray-bounds=]
     5 |     __builtin_strncpy(p, p, 1);
       |     ^~~~~~~~~~~~~~~~~~~~~~~~~~
<source>: In function 'baz':
<source>:11:10: note: 'c' declared here
    11 |     char c[0];
       |          ^
diff mbox series

Patch

diff --git a/gcc/langhooks.cc b/gcc/langhooks.cc
index 61f2b676256..7a2a66b3c39 100644
--- a/gcc/langhooks.cc
+++ b/gcc/langhooks.cc
@@ -395,6 +395,8 @@  lhd_print_error_function (diagnostic_context *context, const char *file,
 	  else
 	    fndecl = current_function_decl;
 
+	  fndecl = DECL_ORIGIN(fndecl);
+
 	  if (TREE_CODE (TREE_TYPE (fndecl)) == METHOD_TYPE)
 	    pp_printf
 	      (context->printer, _("In member function %qs"),
@@ -439,6 +441,7 @@  lhd_print_error_function (diagnostic_context *context, const char *file,
 		}
 	      if (fndecl)
 		{
+		  fndecl = DECL_ORIGIN(fndecl);
 		  expanded_location s = expand_location (*locus);
 		  pp_comma (context->printer);
 		  pp_newline (context->printer);
diff --git a/gcc/testsuite/gcc.dg/pr102061.c b/gcc/testsuite/gcc.dg/pr102061.c
new file mode 100644
index 00000000000..dbdd23965e7
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr102061.c
@@ -0,0 +1,35 @@ 
+/* { dg-do compile } */
+/* { dg-options "-Wall -O2" } */
+/* { dg-message "inlined from 'bar'" "" { target *-*-* } 0 } */
+/* { dg-excess-errors "" } */
+
+static inline void
+foo (char *p)
+{
+  __builtin___memcpy_chk (p, "abc", 3, __builtin_object_size (p, 0));
+}
+static void
+bar (char *p) __attribute__((noinline));
+static void
+bar (char *p)
+{
+  foo (p);
+}
+void f(char*) __attribute__((noipa));
+char buf[2];
+void
+baz (void) __attribute__((noinline));
+void
+baz (void)
+{
+  bar (buf);
+  f(buf);
+}
+
+void f(char*)
+{}
+
+int main(void)
+{
+    baz();
+}