On Nov 20, 2024, Richard Biener <richard.guent...@gmail.com> wrote:
> On Sun, Nov 3, 2024 at 11:27 PM Lewis Hyatt <lhy...@gmail.com> wrote:
>> While testing this with --enable-checking=rtl, I came across one place in
>> final.cc that seems to be a (currently) harmless misuse of RTL:
>>
>> set_cur_block_to_this_block:
>> if (! this_block)
>> {
>> if (INSN_LOCATION (insn) == UNKNOWN_LOCATION)
>> continue;
>> else
>> this_block = DECL_INITIAL (cfun->decl);
>> }
>>
>> In this part of reemit_insn_block_notes(), the insn variable could actually
>> be a NOTE and not an INSN. In that case, INSN_LOCATION() shouldn't be
>> called on it. It works fine currently because the field is properly accessed
>> by XINT() either way. (For an INSN, it is a location, but for a NOTE, it is
>> the note type enum). Currently, if insn is a NOTE, the comparison must
>> always be false because the note type is not equal to
>> 0==UNKNOWN_LOCATION. Once locations and ints are differentiated, this line
>> leads to a checking failure, which I resolved by checking for the NOTE_P
>> case before calling INSN_LOCATION.
>> if (! this_block)
>> {
>> - if (INSN_LOCATION (insn) == UNKNOWN_LOCATION)
>> + if (!NOTE_P (insn) && INSN_LOCATION (insn) == UNKNOWN_LOCATION)
>> continue;
>> else
> I think you instead want
> if (NOTE_P (insn)
> || INSN_LOCATION (insn) == UNKNOWN_LOCATION)
> continue;
> but the whole if (! this_block) block doesn't make sense to me ... I think
> we only get here for NOTE_P via
> case NOTE_INSN_BEGIN_STMT:
> case NOTE_INSN_INLINE_ENTRY:
> this_block = LOCATION_BLOCK (NOTE_MARKER_LOCATION (insn));
> goto set_cur_block_to_this_block;
> so possibly a !this_block case should be made explicit there, by checking
> NOTE_MARKER_LOCATION for UNKNOWN_LOCATION. CCing Alex who
> might know.
I don't recall for sure, but I suspect my assumption back then was that
this_block taken from the marker would never or hardly ever be NULL, but
that it would be good to check before attempting to change the scope to
a NULL block.
I may have missed the distinction between INSN_LOCATION and RTL_LOCATION
there (my notes from back then don't even mention this, only the taking
of a location from the marker, which the case does), but your proposed
change looks reasonable. Leaving that bit alone, moving the label down
and adding a test before the goto would also be fine.
I'm not entirely sure what the best thing to do in case the note doesn't
carry location information, or the referenced block is missing or
however else it could be NULL. Staying in the previous scope is
somewhat sensible, but it amounts to silently dropping debug
information; a visible change of scope might be preferred, even if it's
to the whole-function scope, lacking a more sensible one.
--
Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/
Free Software Activist GNU Toolchain Engineer
Learn the truth about Richard Stallman at https://stallmansupport.org/
