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

[Ben Woodard <woodard@redhat.com>]

I do not believe applying the fundamental type folding only to self comparison will meet our needs. The real reason that self comparison is important to us is we need to throw away the binary and then save the abixml. Then later on we can compare the abixml to a new build to find out if something happened that broke ABI. Thus for our needs, we will not be in a self comparison operation at the time when we come across this in a non-testing environment. I believe that the ordering of the TUs that make up the binary could make this problem appear, thus I do not think it will serve our purposes.

I know that C doesn’t have a ODR but I’m fairly certain that code along the lines of what is in btrfs-progs will have portability problems between architectures. Therefore, I think that programs like this should be fixed. I’m start a discussion with the btrfs guys and explain the problem. I can imagine a possible reason for this problem in btrfs-progs though, it could be caused by the need to read a FS created on one arch on a machine of another arch. I ran into that a few times many years ago and worked with upstream to fix them. (Ia64 era IIRC)

However, the fact that libabigail catches this problem is far too late. At the very least I’d say the compiler should warn about issues like this. We should talk to the compiler guys and see if we can have them create a warning about this situation. Dodji could you start a discussion with our compiler team on this and I will weigh in on it.

As for libabigail, this problem is too subtle for a normal user to understand given the current output and so we need to do something. When reading a corpus of a binary when you come across a case where two different typedefs from two different TUs have different types but the ABI of the fundamental type is compatible, you should emit a warning saying something like: “two typedefs types from different TUs have conflicting types. ABI comparisons cannot be reliably done. file1.c:<line no> and file2.c:<line no>“. So basically at the place where you currently fold  the fundamental integral types in this patch if they are ABI compatible, print a warning instead. That will let me know what the problem is when I do a test run and I will report it to the upstream package rather than you. I’ll try to fix the world and it won’t be your problem.

Maybe have a big comment near the logic in the DWARF reader or where this is detected that explains the decision for the next guy. 

So in summary: 
1) Revert or replace this patch and add warning to libabigail. That should satisfy both me and Giuliano. 
2) Start a discussion with the GCC folk about adding a warning. I’ll follow up.
3) I’ll use the GCC warning discussion to bring this up with upstream btrfs people and possibly suggest a patch to fix their stuff.
4) I’ll do the same with any other packages that have a similar problem.

-ben

> On Aug 18, 2022, at 9:29 AM, Dodji Seketeli <dodji@seketeli.org> wrote:
> 
> Giuliano Procida <gprocida@google.com> a écrit:
> 
>> Sorry, I have to be brief...
> 
> No problem.
> 
> [...]
> 
>>> 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.
> 
> This is interesting.
> 
> Historically, I chose to analyse binaries rather than source code
> precisely because I wanted to compare the ABIs of the binaries directly,
> architecture per architecture, rather than trying to infer what API
> change might incur an ABI change.  The main assumption is that we do
> *HAVE* access to the actual binaries.  And what I really cared about was
> actual ABI changes, not API changes.
> 
> Doing the API compatibility verification came afterwards, in a "nice to
> have manner", from the request of users over time, like icing on the
> cake, so to speak.  The core of what I wanted really was ABI comparison.
> 
> It's funny to see how the API side of the requirement got stronger over
> time.
> 
> Anyway, I think I get your point.
> 
> [...]
> 
>>>>> 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.
> 
> Of course.  But keep some information all the way can be more costly
> than trimming them off early, if we know we don't need them.  It's a
> tradeoff that seems clear in my mind.
> 
>> Alternatively, isn't this sort of thing exactly what the suppression logic in
>> abidiff is supposed to be used for?
> 
> Self-comparing the IR from a binary against it's abixml counterpart is
> supposed to be done without any suppression specification applied.
> 
> OK, so here is what I am proposing.
> 
> Either I disable this thing altogether (namely, saying that int, short
> int, long int and long long int are the same if the types have the same
> size; note that other integral types are not touched by this) and give
> up on the self-comparison check failure of the btrfs-progs package or I can
> use this "abi-only-comparison" only when we do self-comparison check,
> i.e, when we do abidw --abidiff and abipkgdiff --self-check.
> 
> I have a candidate patch for the later and the former is even easier to
> do.
> 
> @Ben and @Giuliano, what would you prefer?
> 
> Cheers,
> 
> -- 
>        Dodji
>