Re: [PATCH] ir, dwarf-reader: Peel const-qualifier from const this pointers

[Ben Woodard <woodard@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4302 bytes --]

On Wed, Mar 6, 2024 at 7:15 AM Dodji Seketeli <dodji@redhat.com> wrote:

> Hello Ben,
>
> Ben Woodard <woodard@redhat.com> a écrit:
>
> > I believe that I have a bug open on that one.
>
> Yes, there are a series of bugs of yours related to that, I think.  I
> need to re-test them to see where we stand today.  A number of other
> things have been fixed since then, so I might need to take another
> view at the whole thing.
>
> > Are you addressing it or did you independently find the problem?
>
> I was looking at something else (abidb), but I had your issues in mind
> too when I stumbled upon this.
>
> > This is also something that causes false positive ABI problems between
> > different toolchains.
>
> Exactly.  This patch reduces the distance between the output of g++ and
> clang, as shown by the change that the patch has on the test result
> tests/data/test-diff-dwarf/test42-PR21296-clanggcc-report0.txt.
>
>
> > The question for me though, is this the “right” fix? I remember
> > reasoning through the DWARF and wondering if a “this” pointer really
> > should be const indicating that it could be a bug in the compilers
> > when it is not. This makes me wonder if peeling the const pointer is
> > the right solution or if it is just expedient given the current state
> > of the compilers.
>
> That's up for debate, I would say :-) (sorry).


Yeah I know. I couldn’t puzzle through that one. I think that we should
probably first bring it up to the compiler guys and get their take on it. I
kind of think that it is a compiler bug and if it is not, and there is a
good reason behind it. Then we might learn something that we should take to
dwarf-discuss. Based on you explanation which was clearer than my
understanding, I think that the compiler guys are a the right first place
to discuss this rather than dwarf-discuss.

>
>
> In all the binaries I looked at, the const qualifier is added only on
> the type of the this-pointer parameter carried by the concrete inlined
> instance of a member function.  The abstract instance that actually
> reflects what the user code is doesn't have that const qualifier.
>

This is the part that kind of gives me misgivings. I fear that when we get
to the point of properly handling inline functions, this might come back to
haunt us.

Here is a potential scenario which kind of concerns me and I agree upfront
that it may be bordering on function semantics rather than ABI.

Let’s say that a programmer changes the optimization flags and a function
moves from being inlined within an app due to some optimization opportunity
but also available in the library to being in an object file. Is the app
that contains the compiled in version of the function still ABI compatible
with the new version of library which has the function in a library. One of
the things that we hoped to detect here but never got to before we ran out
of time on the project was ensuring that different optimization flags
didn’t impact ABI in undetectable ways. I will admit that I never got a
handle on that part of the project.

One of the things that we worried about in the ABI project here is embedded
concrete instances of  functions which are inlined ending up being
semantically different than newer versions in a later version of the
library. The best that I could offer was we could check function prototypes
but there was no way to detect semantic differences for inline functions.


> In other words, the const qualifier is artificially added by the
> compiler to the inlined concrete instance of the function.
>
> Ultimately, that const qualifier has no material impact on the ABI, as
> far as I can tell.
>
> So rather than having to spend energy to try and figure out where it
> comes from and reason where there, I thought that stripping it off
> might do as well.


I kind of agree with some not well defined misgivings but I think that we
should discuss with the compiler people it and pay attention to see if it
has unforeseen implications when we get to analyzing inline functions.

-ben


>
>
> I hope this explains my reasoning a bit better.
>
> Thanks for following up.
>
> [...]
>
> Cheers,
>
> --
>                 Dodji
>
>