Managing bugfixes that cause struct-layout changes

Hi,

We at Sony have a particularly strong interest in not breaking object file compatibility from release to release. So when a bug exists in release X, which gets fixed in release Y, where that bugfix is an ABI-breaking change, we generally have to weigh the pluses of fixing the bug against the minuses of breaking our object compatibility goal. The impact of object file incompatibility is quite bad for us, and so we likely are more concerned than most toolchain providers about this issue. But that said, I would imagine there are others out there that have some concerns along these lines. I’m curious to hear about preferred methods of dealing with this.

A straightforward solution is for us simply to guard an ABI-breaking bugfix with our PS4-triple. Conceptually:

if (TargetIsPS4) {

// old way, with known bug, to retain object compatibility

} else {

// new way, that fixes bug, but breaks object compatibility

}

But if others are interested in retaining the compatibility, then instead, something like the following may be preferred:

if (TargetUsesBuggyApproachX) {

// old way, with known bug, to retain object compatibility

} else {

// new way, that fixes bug, but breaks object compatibility

}

where the concept of “Target Uses Buggy Approach X” is implemented by, for example, adding another single-bit bitfield to the TargetInfo class, and initializing it appropriately for different targets.

To be more specific, a current example of an ABI-breaking issue for us is PR22279:

https://llvm.org/bugs/show_bug.cgi?id=22279

As described there, a bugfix in the handling of alignment attributes (and alignas()) for enums causes a change in object layout. For our customers, and hence for us, the fix in proper alignment of enum types in these cases isn’t as important as retaining object compatibility. So we need to suppress that fix. It’s easy enough for us to suppress the change by checking whether the target is PS4, but adding a bit to TargetInfo like:

unsigned IgnoreEnumTypeAlignment : 1;

seems better. I’m appending a patch on clang below, that shows this approach concretely.

Or possibly there is some other preferred approach? I’m curious to hear people’s opinions.

Thanks,

-Warren

Shouldn’t a diagnostic be given if the path returning the incorrect calculation would return a result that disagrees from the correct calculation?

Yes, I agree a diagnostic is a good idea.

That said, I wasn’t so much proposing the specifics of that example-patch as the precise fix/change to make, as I was using it to concretely demonstrate to concept of how I was thinking of adding a flag to the TargetInfo class, and then making use of it. I’m wondering do others care about an object-compatibility issue like this (in which case guarding it in this more generic way seems sensible), or would it be better to just guard it directly with the PS4-triple?

Thanks,

-Warren

Yes, I agree a diagnostic is a good idea.

That said, I wasn't so much proposing the specifics of that example-patch
as the precise fix/change to make, as I was using it to concretely
demonstrate to concept of how I was thinking of adding a flag to the
TargetInfo class, and then making use of it. I'm wondering do others care
about an object-compatibility issue like this (in which case guarding it in
this more generic way seems sensible), or would it be better to just guard
it directly with the PS4-triple?

I'm not so sure we need to burn a bit for this sort of thing. We already
have stuff like honorsRevision0_98() which is purely triple based.

Us on the clang-cl side care deeply about compatibility between clang and
MSVC to the point where we will mimic their semantic bugs. However, we are
not interested in mimicing bugs in older versions of clang.

TBH, I'm very curious how far you want to take this. Would every fix to
ItaniumMangle.cpp be wrapped in if () logic?

I’m not so sure we need to burn a bit for this sort of thing. We already

have stuff like honorsRevision0_98() which is purely triple based.

Makes sense. The approach of just checking the triple in a method (analogous to honorsRevision0_98()) seems quite sufficient, and doesn’t burn the bit. Thanks.

TBH, I’m very curious how far you want to take this. Would every fix to

ItaniumMangle.cpp be wrapped in if () logic?

In general, we’d want to look carefully at any fix in mangling. Some (but very few) are guaranteed to be only locally visible, and so not cause trouble for us. Others are enough of a corner case, that we’re willing to “take the risk” and accept them. As I said at the start of my first post on this, we have more reason to worry about these things than most toolchain providers, so we tend to look carefully at those fixes. But I do expect most others to take fixes like that, without much concern.

Thanks,

-Warren

Hi all,

As I mentioned when I first asked about this, we at Sony are probably more interested in compatibility issues like this than most toolchain providers. On thinking a bit more about it, the particular issue that motivated my general question about struct-layout changes, PR22279:

https://llvm.org/bugs/show_bug.cgi?id=22279

As described there, a bugfix in the handling of alignment attributes (and alignas())

for enums causes a change in object layout.

is obscure enough that I wouldn’t be at all surprised if no one else is concerned about it. If that’s the case, then we’ll likely just deal with this internally, rather than adding a method like ‘ignoreEnumTypeAlignment()’ in the public sources.

The bottom line: Are there others out there who are concerned about this particular issue of alignment changing for some enums that creates the possibility of a struct-layout change? If not, we will deal with this locally.

Thanks,

-Warren