On Wed, 2024-08-21 at 16:05 -0400, Jason Merrill wrote:
> On 8/21/24 3:34 PM, Arsen Arsenović wrote:
> > Jason Merrill <[email protected]> writes:
> >
> > > On 8/21/24 1:52 PM, Arsen Arsenović wrote:
> > > > For clarity, here's the entire split-up patch I intend to push,
> > > > if it
> > > > looks OK. Tested on x86_64-pc-linux-gnu.
> > > > I've renamed the field we've discussed and also a few
> > > > parameters that
> > > > refer to 'kw' to be less specific. The code is functionally
> > > > identical.
> > > > OK for trunk?
> > > > TIA, have a lovely day.
> > > > ---------- >8 ----------
> > > > This patch improves the EXPR_LOCATION associated with parsed
> > > > RETURN_EXPRs so that they can be used in diagnostics later.
> > > > This change
> > > > also happened to un-suppress an analyzer false-negative that
> > > > was
> > > > happening because the location of RETURN_EXPR was entirely
> > > > within the
> > > > NULL macro, which was defined in a system header. PR
> > > > analyzer/116304.
> > >
> > > The PR number should be on its own line, and in the subject line.
> >
> > It isn't a total fix for that PR, that's why I didn't name it as
> > part of
> > the changelog and subject line, but to be fair it would not be
> > wrong to
> > put it there anyway, as it is related. Will reword.
> >
> > > > gcc/cp/ChangeLog:
> > > > * cp-tree.h (finish_return_stmt): Add optional
> > > > location_t
> > > > parameter, defaulting to input_location.
> > > > * parser.cc (cp_parser_jump_statement): Improve return
> > > > and
> > > > co_return locations so that they span their entire
> > > > statements.
> > > > * semantics.cc (finish_return_stmt): Use the new
> > > > stmt_loc
> > > > parameter in place of input_location.
> > > > gcc/testsuite/ChangeLog:
> > > > * c-c++-common/analyzer/inlining-4-multiline.c: Adjust
> > > > locations
> > > > in diagnostics.
> > >
> > > This doesn't look like a location adjustment, but removing
> > > testing of expected
> > > output. If the output changed, please change the test to check
> > > the new output
> > > rather than not at all.
> >
> > This is the new output - the diagnostics no longer expand that
> > macro,
> > since the location is not wholly contained within it. The relevant
> > part
> > of the diagnostics after the change is:
> >
> > --8<---------------cut here---------------start------------->8---
> > |
> > 'const char* inner(int)': event 5 (depth 3)
> > |
> > | return NULL;
> > |
> > --8<---------------cut here---------------end--------------->8---
> >
> > ... as opposed to (excuse the quote difference - the former was
> > pulled
> > from g++.log and the latter from a manual invocation):
> >
> > --8<---------------cut here---------------start------------->8---
> > |
> > ‘const char* inner(int)’: event 5 (depth 3)
> > |
> > |/home/arsen/gcc-mine/gcc/testsuite/c-c++-
> > common/analyzer/../../gcc.dg/analyzer/analyzer-decls.h:7:14:
> > | #define NULL nullptr
> > | ^~~~~~~
> > | |
> > | (5) ...to here
> > /home/arsen/gcc-mine/gcc/testsuite/c-c++-common/analyzer/inlining-
> > 4-multiline.c:14:12: note: in expansion of macro ‘NULL’
> > | return NULL;
> > | ^~~~
> > |
> > --8<---------------cut here---------------end--------------->8---
> >
> > ... presumably, the diagnostics chose to elide those bits of output
> > due
> > to the new location covering the entire line (and hence not being
> > too
> > informative) - but I haven't debugged that (as I assumed the
> > diagnostic
> > code is DTRT now).
>
> It seems weird to lose the "...to here" marking on the return. Any
> thoughts, David?
Sorry for missing this earlier.
With the patch applied, I see:
$ ./xg++ -B. -S -fanalyzer -O2
../../src/gcc/testsuite/c-c++-common/analyzer/inlining-4-multiline.c
../../src/gcc/testsuite/c-c++-common/analyzer/inlining-4-multiline.c: In
function ‘char outer(int)’:
../../src/gcc/testsuite/c-c++-common/analyzer/inlining-4-multiline.c:27:23:
warning: dereference of NULL ‘0’ [CWE-476] [-Wanalyzer-null-dereference]
27 | return *middle (flag); /* { dg-warning "dereference of NULL"
"warning" } */
| ^
‘char outer(int)’: events 1-2
│
│ 25 | outer (int flag)
│ | ^~~~~
│ | |
│ | (1) entry to ‘outer’
│ 26 | {
│ 27 | return *middle (flag); /* { dg-warning "dereference of NULL"
"warning" } */
│ | ~
│ | |
│ | (2) inlined call to ‘middle’ from ‘outer’
│
└──> ‘const char* middle(int)’: event 3
│
│ 21 | return inner (flag);
│ | ^
│ | |
│ | (3) inlined call to ‘inner’ from ‘middle’
│
└──> ‘const char* inner(int)’: event 4
│
│ 13 | if (flag)
│ | ^~
│ | |
│ | (4) following ‘true’ branch (when ‘flag != 0’)...
─>─┐
│ |
│
│
‘const char* inner(int)’: event 5
│
│ |
│
│
|┌───────────────────────────────────────────────────────┘
│ 14 |│ return NULL;
│
<─────────────┘
│
‘char outer(int)’: event 6
│
│ 27 | return *middle (flag); /* { dg-warning "dereference of NULL"
"warning" } */
│ | ^
│ | |
│ | (6) ⚠️ dereference of NULL ‘<unknown>’
│
and in particular this weird output for event 5:
‘const char* inner(int)’: event 5
│
│ |
│
│
|┌───────────────────────────────────────────────────────┘
│ 14 |│ return NULL;
│
<─────────────┘
I did some poking at this in the debugger (via a breakpoint on
path_summary::path_summary).
Looking at event (5) (with idx == 4)'s location_t:
(gdb) p cur_event_range->m_richloc.m_ranges.m_embedded[0]
$13 = {m_loc = 4611686018427387937, m_range_display_kind =
SHOW_RANGE_WITH_CARET,
m_label = 0x526e358, m_highlight_color = 0x0}
(gdb) p /x cur_event_range->m_richloc.m_ranges.m_embedded[0]
$14 = {m_loc = 0x4000000000000021, m_range_display_kind = 0x0, m_label
= 0x526e358,
m_highlight_color = 0x0}
Looks like this is an ad-hoc location (64-bit):
(gdb) call inform (0x4000000000000021, "")
In function ‘const char* inner(int)’,
inlined from ‘const char* middle(int)’ at
../../src/gcc/testsuite/c-c++-common/analyzer/inlining-4-
multiline.c:21:16,
inlined from ‘char outer(int)’ at ../../src/gcc/testsuite/c-c++-
common/analyzer/inlining-4-multiline.c:27:18:
14 | return NULL;
Looking up the ad-hoc data for 0x4000000000000021:
(gdb) p line_table->m_location_adhoc_data_map.data[0x21]
$16 = {locus = 2368512, src_range = {m_start = 2368512, m_finish =
4611686018427387897},
data = 0x7fffea650958, discriminator = 0}
The inlining info is in "data"; looking at the caret/start/finish
locations:
(gdb) call inform (2368512, "")
../../src/gcc/testsuite/c-c++-common/analyzer/inlining-4-multiline.c:
In function ‘char outer(int)’:
14 | return NULL;
| ^
(gdb) call inform (4611686018427387897, "")
In file included from ../../src/gcc/testsuite/c-c++-
common/analyzer/inlining-4-multiline.c:7:
7 | #define NULL nullptr
| ^~~~~~~
../../src/gcc/testsuite/c-c++-common/analyzer/inlining-4-
multiline.c:14:12: note: in expansion of macro ‘NULL’
14 | return NULL;
| ^~~~
So the issue is that the "start" and "caret" parts of the location_t
are at the "r" of "return" in inlining-4-multiline.c line 14, but the
"end" part of the location_t is on the NULL token expanded as part of a
macro expansion, presumably created by this part of the patch:
+ /* A location spanning the whole statement (up to ';'). */
+ auto stmt_loc = make_location (token->location,
+ token->location,
+ input_location);
+
Hence the location_t spans two different source files, and diagnostic-
show-locus.cc rejects attempting to draw an underline for such a
location_t, and hence the text label for the underline ("...to here")
doesn't get printed either.
I'm not sure if the location_t value being created by the patch is
"wrong" or whether diagnostic-show-locus.cc could/should have a
workaround for such cases.
Dave
>
> > > > * c-c++-common/analyzer/malloc-paths-9-noexcept.c:
> > > > Ditto.
> > >
> > > ...like you do properly in this test.
> > >
> > > > * c-c++-common/analyzer/malloc-CWE-401-example.c:
> > > > Accept the
> > > > new warning on line 34 (fixed false negative).
> > >
> > > I'd think the new dg-warning could replace the obsolete TODO?
> >
> > I left it because the TODO still exists for C (which wasn't fixed
> > in
> > this patch - hence the { target c++ }). I can clarify it like:
> >
> > /* TODO: should complain that "buf" is leaked on this path in C
> > also. */
> >
> > ... which should be more informative.
>
> Sounds good.
>
> Jason
>
