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

[Jason Merrill <jason@redhat.com>]

On 1/31/24 21:09, Lewis Hyatt wrote:
> On Wed, Jan 31, 2024 at 03:18:01PM -0500, Jason Merrill wrote:
>> 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?
> 
> Thanks, yes, that will work too, it just needs a few changes to
> c-family/c-pch.cc to set the location there and then increment it
> after. Patch which does that is attached. (This is a new one based on
> master, not incremental to the prior patch.) The testcase does not require
> any changes this way, and bootstrap + regtest looks good.

OK, thanks.

Jason