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

[Dodji Seketeli <dodji@seketeli.org>]

Hello,

Ben Woodard <woodard@redhat.com> a écrit:

> 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.

OK.

> 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.

This is a well known problem in the compiler space.  G++ now has the
-Wno-odr warning to detect ODR violation and emit a warning.  But that's
for C++.

For C however, I am not sure.


> Dodji could you start a discussion with our compiler team on this and
> I will weigh in on it.

Yeah, I'll see what I can do.

> 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.

Emitting a comment that makes sense would be nice, indeed.  But I'll
need to tackle this separately because it's not exactly a one
liner. There can be a lot of types being in that case, just because they
happen to use a type that exhibits the problem.  We need to emit the
comment just for the type that is at the root cause of the problem.  So
it takes a little bit of thinking, I think.

Also, I might have a way to handle this in a way that enable us to
represent those ODR-violating types regardless.  I don't like how
libabigail is fragile here.  But I need to play with the idea a little.
And I might be wrong.  I think this topic needs some more hashing out.

> 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. 


OK, I have just done this.  The patch is https://sourceware.org/pipermail/libabigail/2022q3/004666.html.

> 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.

As I said above, I need to think about this a little bit more.

> 4) I’ll do the same with any other packages that have a similar problem.

Yeah.  The problem though is that it needs very careful debugging to
understand the problem.  That is where your point about libabigail
emitting a warning makes sense.

Thank you for your handling this.

[...]

Cheers,

-- 
		Dodji