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

[Jason Merrill <jason@redhat.com>]

On 1/30/24 21:49, Lewis Hyatt wrote:
> 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!

Can we use the line of the #include, as the test expects, rather than 
the following line?

Jason