Re: [PATCH] c-family: Fix ICE with large column number after restoring a PCH [PR105608]

[Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE>]

Hi Lewis,

> On Fri, Jan 26, 2024 at 04:16:54PM -0500, Jason Merrill wrote:
>> On 12/5/23 20:52, Lewis Hyatt wrote:
>> > Hello-
>> > 
>> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105608
>> > 
>> > There are two related issues here really, a regression since GCC 11 where we
>> > can ICE after restoring a PCH, and a deeper issue with bogus locations
>> > assigned to macros that were defined prior to restoring a PCH.  This patch
>> > fixes the ICE regression with a simple change, and I think it's appropriate
>> > for GCC 14 as well as backport to 11, 12, 13. The bad locations (wrong, but
>> > not generally causing an ICE, and mostly affecting only the output of
>> > -Wunused-macros) are not as problematic, and will be harder to fix. I could
>> > take a stab at that for GCC 15. In the meantime the patch adds XFAILed
>> > tests for the wrong locations (as well as passing tests for the regression
>> > fix). Does it look OK please? Bootstrap + regtest all languages on x86-64
>> > Linux. Thanks!
>> 
>> OK for trunk and branches, thanks!
>>
>
> Thanks for the review! That is all taken care of. I have one more request if
> you don't mind please... There have been some further comments on the PR
> indicating that the new xfailed testcase I added is failing in an unexpected
> way on at least one architecture. To recap, the idea here was that
>
> 1) libcpp needs new logic to be able to output correct locations for this
> case. That will be some new code that is suitable for stage 1, not now.
>
> 2) In the meantime, we fixed things up enough to avoid an ICE that showed up
> in GCC 11, and added an xfailed testcase to remind about #1.
>
> The problem is that, the reason that libcpp outputs the wrong locations, is
> that it has always used a location from the old line_map instance to index
> into the new line_map instance, and so the exact details of the wrong
> locations it outputs depend on the state of those two line maps, which may
> differ depending on system includes and things like that. So I was hoping to
> make one further one-line change to libcpp, not yet to output correct
> locations, but at least to output one which is the same always and doesn't
> depend on random things. This would assign all restored macros to a
> consistent location, one line following the #include that triggered the PCH
> process. I think this probably shouldn't be backported but it would be nice
> to get into GCC 14, while nothing critical, at least it would avoid the new
> test failure that's being reported. But more generally, I think using a
> location from a totally different line map is dangerous and could have worse
> consequences that haven't been seen yet. Does it look OK please? Thanks!

FWIW, I've tested this (the initial) version of this patch on
sparc-sun-solaris2.11 (PASSes as before) and i386-pc-solaris2.11 (PASSes
now unlike before).

Thanks.
        Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University