[clang][Sema] theoretical ABI break in LLVM 20.1.1

[isuckatcs]

In LLVM 20.1.0 we have the following Sema class.

class Sema final : public SemaBase {
  ...

private:
  /// Function or variable declarations to be checked for whether the deferred
  /// diagnostics should be emitted.
  llvm::SmallSetVector<Decl *, 4> DeclsToCheckForDeferredDiags;

  /// Map of current shadowing declarations to shadowed declarations. Warn if
  /// it looks like the user is trying to modify the shadowing declaration.
  llvm::DenseMap<const NamedDecl *, const NamedDecl *> ShadowingDecls;

  ...
};

In LLVM 20.1.1 this definition is changed the way below.

class Sema final : public SemaBase {
  ...

public:

  ...

  /// Function or variable declarations to be checked for whether the deferred
  /// diagnostics should be emitted.
  llvm::SmallSetVector<Decl *, 4> DeclsToCheckForDeferredDiags;

private:
  

  /// Map of current shadowing declarations to shadowed declarations. Warn if
  /// it looks like the user is trying to modify the shadowing declaration.
  llvm::DenseMap<const NamedDecl *, const NamedDecl *> ShadowingDecls;

  ...
};

In other words, DeclsToCheckForDeferredDiags is made public in 20.1.1, when it was private in 20.1.0.

Until C++23 the C++ standard said the following about this.

[Note: Non-static data members of a (non-union) class with the same access control (11.9) and non-zero
size (6.7.2) are allocated so that later members have higher addresses within a class object. The order of
allocation of non-static data members with different access control is unspecified.

N4849 § 11.4 19

With C++23 however this restriction was removed though.

[Note 8 : Non-variant non-static data members of non-zero size (6.7.2) are allocated so that later members
have higher addresses within a class object (7.6.9).

N4950 § 11.4.1 20

LLVM is still compiled targeting the C++17 standard, which means, the first quote of the standard applies and compilers have the right to change the field order of Sema IIUC.

Normally this ABI break is the part of libclangSema.a, however we also have libclang-cpp.so, which collects all of the static libraries of clang and makes it possible to link against them dynamically. If an application is linked against clang dynamically, then according to the standard libclang-cpp.so.20.1.1 is not compatible with libclang-cpp.so.20.1.

I'm not sure if there are compilers taking advantage of this, but if they do, the resulting libclang-cpp.so might not be ABI compatible with it's previous release, as fields might have different offsets within the object.

[llvmbot]

@llvm/issue-subscribers-clang-frontend

Author: None (isuckatcs)

In LLVM 20.1.0 we have the following `Sema` class. ```c++ class Sema final : public SemaBase { ...

private:
/// Function or variable declarations to be checked for whether the deferred
/// diagnostics should be emitted.
llvm::SmallSetVector<Decl *, 4> DeclsToCheckForDeferredDiags;

/// Map of current shadowing declarations to shadowed declarations. Warn if
/// it looks like the user is trying to modify the shadowing declaration.
llvm::DenseMap<const NamedDecl *, const NamedDecl *> ShadowingDecls;

...
};

In LLVM 20.1.1 this definition is changed the way below.

```c++
class Sema final : public SemaBase {
  ...

public:

  ...

  /// Function or variable declarations to be checked for whether the deferred
  /// diagnostics should be emitted.
  llvm::SmallSetVector<Decl *, 4> DeclsToCheckForDeferredDiags;

private:
  

  /// Map of current shadowing declarations to shadowed declarations. Warn if
  /// it looks like the user is trying to modify the shadowing declaration.
  llvm::DenseMap<const NamedDecl *, const NamedDecl *> ShadowingDecls;

  ...
};

In other words, DeclsToCheckForDeferredDiags is made public in 20.1.1, when it was private in 20.1.0.

Until C++23 the C++ standard said the following about this.
> [Note: Non-static data members of a (non-union) class with the same access control (11.9) and non-zero
> size (6.7.2) are allocated so that later members have higher addresses within a class object. The order of
> allocation of non-static data members with different access control is unspecified.
>
> N4849 § 11.4 19

With C++23 however this restriction was removed though.
> [Note 8 : Non-variant non-static data members of non-zero size (6.7.2) are allocated so that later members
> have higher addresses within a class object (7.6.9).
>
> N4950 § 11.4.1 20

LLVM is still compiled targeting the C++17 standard, which means, the first quote of the standard applies and compilers have the right to change the field order of Sema IIUC.

Normally this ABI break is the part of libclangSema.a, however we also have libclang-cpp.so, which collects all of the static libraries of clang and makes it possible to link against them dynamically. If an application is linked against clang dynamically, then according to the standard libclang-cpp.so.20.1.1 is not compatible with libclang-cpp.so.20.1.

I'm not sure if there are compilers taking advantage of this, but if they do, the resulting libclang-cpp.so might not be ABI compatible with it's previous release, as fields might have different offsets within the object.

[isuckatcs]

I was wrong in that this is not an issue in libclang.so, but an issue in libclang-cpp.so, so I updated the wording accordingly.

[shafik]

CC @tstellar is this a "real" ABI break? I am hearing from some folks we have an ABI validator tool, is this something we run on reach release?

[tstellar]

We do have an ABI validator that we run, but it doesn't seem to work any more. It hasn't been catching obvious breaks and I don't think it ever worked for checking enum values.

[Endilll]

I'm not sure if there are compilers taking advantage of this, but if they do, the resulting libclang-cpp.so might not be ABI compatible with it's previous release, as fields might have different offsets within the object.

According to P1847R4, only EDG implemented that, but none of their customers actually used the ability to reorder data members that have different access specifiers. I agree that theoretically this is an ABI break, but I doubt that layout of Sema has actually changed in 20.1.1.

[isuckatcs]

Here is another interesting change, I found.

LLVM 20.1.0:

class MachineInstr {
public:
  bool isIndirectBranch(QueryType Type = AnyInBundle) const {
    return hasProperty(MCID::IndirectBranch, Type);
  }
};

LLVM 20.1.1:

class MachineInstr {
public:
  bool isIndirectBranch(QueryType Type = AnyInBundle,
                        bool IncludeJumpTable = true) const {
    return hasProperty(MCID::IndirectBranch, Type) &&
           (IncludeJumpTable || !llvm::any_of(operands(), [](const auto &Op) {
              return Op.isJTI();
            }));
  }
};

Notice how the signature of the function has changed. Since it is defined inside the class body, it should be inlined in the user code, and even if the compiler decides not to inline it, the definition should always be present in the translatation unit, where the method is called.

Some guides however still not recommend doing something like this. The KDE Binary Compatibility Policy states the following:

You can ... change an inline function or make an inline function non-inline if it is safe that programs linked with the prior version of the library call the old implementation. This is tricky and might be dangerous. Think twice before doing it.

I'm not sure how the program can call the new implementation, as IIUC, every TU, where the symbol of the method is used should contain the definition.

The only concern that might arise is that having one definition in the consumer, and loading the new version of the library, that contains a completely different definition of the same symbol, will lead to ODR violation, as it is also pointed out by this other C++ ABI stability Guidelines, I found.

Changing the implementation of some methods in a somewhat incompatible manner with the previous implementation might cause logical errors in the code. This can happen because some of the previous implementations (possibly conflicting with the new one) of those functions can be inlined into the application. To make it safe, the old and new implementations of the same function must be able to work together. Besides that, even if it usually works fine, theoretically, having two different implementations of the same function inside one executable is a violation of the one definition rule.

IIRC, an ODR violation is condiered UB.