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

[Lewis Hyatt <lhyatt@gmail.com>]

On Thu, Feb 1, 2024 at 7:24 AM Rainer Orth <ro@cebitec.uni-bielefeld.de> wrote:
>
> 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

Thanks a lot! And sorry for that issue. I will push the updated
version of that patch shortly.