Re: [PATCH 3/3] ir: Consider integral types of same kind and size as equivalent

[Giuliano Procida <gprocida@google.com>]

Sorry, I have to be brief...

On Tue, 16 Aug 2022 at 17:54, Dodji Seketeli <dodji@seketeli.org> wrote:
>
> Giuliano Procida <gprocida@google.com> writes:
>
> > Hi Dodji.
>
> Hello Giuliano,
>
> [...]
>
> > On Sat, 23 Jul 2022 at 00:33, Dodji Seketeli <dodji@seketeli.org> wrote:
> >>
> >> Hello,
> >>
> >> On some platforms, "long int" and "long long int" can have the same
> >> size.  In that case, we want those two types to be equivalent from ABI
> >> standpoint.  Otherwise, through the use of typedefs and pointers, two
> >> structs "C" defined in different translation units where one uses
> >> "long int" in a translation unit and "long long int" in another should
> >> be considered ABI compatible if long int and long long int have the
> >> same size on that platform.
> >
> > While such types may be ABI compatible, they are not API compatible as they
> > impact (at least) C++ overload resolution.
>
> Right.  But as usual with these things (API vs ABI conformance), we try
> to accommodate people's need as much as possible, with a preference with
> ABI conformance when we can't ensure both at the same time.  That's what
> we have done historically, but of course, my stance is not cast in
> stone.  I am open to discussion and change.
>
> In this particular case of /C type/ program, it seems to be that the
> programmers expect the types to be ABI compatible.

I think with so much multi-architecture code about and the difficulty
of (say) ABI
monitoring less common architectures, detecting API changes that will be ABI
breaks on another architecture seems like a big positive.

> C++ of course being strongly type as it is, I understand that we might
> want to be stricter.
>
> > All of char, unsigned char, signed char, int, unsigned, long, etc. are
> > distinct types.
> > Conflating some subsets of these will result in confusing ABI
> > difference reports.
>
> For C++ (or Ada, or in those strongly type languages) I think I
> understand why some change reports might be confusing.  In your mind,
> can we have the issue with C however? (real question).
>
>
> >> Otherwise, that causes spurious type changes that lead to self
> >> comparison change down the road.  For instance, the following command
> >> fails:
> >>
> >>     $ tools/fedabipkgdiff --debug --self-compare -a --from fc36 btrfs-progs
> >
> > Shouldn't any tweaking of behaviour happen with abidiff rather than abidw?
>
>
> I am not sure to understand the question.  This kind of adjustment of
> what is is read from the binary typically tends to happen at the core
> level (either DWARF reader, IR construction/transformation, comparison,
> etc) rather at the level of a specific tool.  Am I missing something
> from what you have in mind?

The earlier the re-interpretation is, the less visible it is and the original
information cannot be recovered.

Alternatively, isn't this sort of thing exactly what the suppression logic in
abidiff is supposed to be used for?

> >
> > We might not end up with stable XML but the finger of blame should be
> > pointed at the btrfs-progs in any case.
>
> I understand and I sympathesize with your point of view.  But just to
> explain where I sit on the matter, there have unfortunately been plenty
> of real life cases where the programs (those written by the app/library
> developers or by the compiler/linker developers) are not what libabigail
> would expect in a perfect world.  So far we try hard to "accommodate"
> when we can, if that can lead to a better experience for libabigail
> users.  But I agree that we shouldn't try harder.  I guess knowing the
> difference is the crux of this art.  So I am open to discussion to try
> to accommodate your point of view too.
>
> >> This patch thus changes the comparison engine of the IR so that the
> >> "short, long and long long" modifiers don't change the result of
> >> comparing integral types that share the same base type when they have
> >> the same size.
> >
> > We don't want this behaviour and can carry a revert patch in AOSP or
> > work a way to disable it that is less likely to cause merge conflicts
> > in the future.
>
> Would it be useful for your case if this behaviour happens just for C
> programs?

We support both ARM32 and ARM64 Android userspace and there are
both C and C++ libraries. So just for ourselves, no.

> > Is there an easy way of putting this under flag control?
>
> I guess so, yes.  But just a flag would not be optimal from a user
> standpoint, would it?

If there is, then it would be easy to disable in a less intrusive way than
carrying a rollback commit in AOSP. We don't actually need the flag
control.

> Thank you for raising this and sorry for the inconvenience.
>
> I hope we resolve this.

Sure. One way or another. :-)

Thanks for the reply.

Giuliano.

> Cheers,
>
> --
>                 Dodji