[RFC] Runtime checks for ABI breaking build of LLVM

Mehdi_Amini3 · November 16, 2016, 6:48pm

Hi all,

An issue that come up from time to time and has cost hours for debug for many of us and users of LLVM is that an assert build isn’t ABI compatible with a release build.

The CMake flags that controls this behavior is LLVM_ABI_BREAKING_CHECKS (

**LLVM_ABI_BREAKING_CHECKS**:STRING

Used to decide if LLVM should be built with ABI breaking checks or not. Allowed values are (default), and . turns on ABI breaking checks in an assertion enabled build. () turns them on (off) irrespective of whether normal (-based) assertions are enabled or not. A version of LLVM built with ABI breaking checks is not ABI compatible with a version built without it.

I propose to add a runtime check to detect when we have incorrectly setup build.

The idea is that every translation unit that includes such header will get a weak definition of a symbol with an initializer calling the runtime check. The symbol name would be different if the ABI break is enabled or not.

The runtime check maintains two boolean to track if it there is in the image at least a translation unit for each value of this flag. If both flags are set the process is aborted.

The cost is one static initializer per DSO (or per image I believe).

A straw-man patch (likely not windows compatible because of weak) is:

diff --git a/llvm/include/llvm/Config/llvm-config.h.cmake b/llvm/include/llvm/Config/llvm-config.h.cmake
index 4121e865ea00…4274c942d3b6 100644
— a/llvm/include/llvm/Config/llvm-config.h.cmake
+++ b/llvm/include/llvm/Config/llvm-config.h.cmake
@@ -80,4 +80,18 @@
/* LLVM version string */
#define LLVM_VERSION_STRING “${PACKAGE_VERSION}”

rnk · November 16, 2016, 7:40pm

I totally support solving this problem, but please don’t use dynamic initialization to do it. That’s everyone’s least favorite feature of libstdc++ iostream.

At least on Windows, we can solve this with #pragma detect_mismatch. https://msdn.microsoft.com/en-us/library/ee956429.aspx

On platforms without such a feature, I’d rather mangle NDEBUG into one of the core LLVM initialization routines than suffer the runtime and code size penalty of initializers.

Mehdi_Amini3 · November 16, 2016, 8:03pm

Why? I know that static initializer aren’t great but don’t we already have hundreds of them? For instance the cl::opt?

Also, is there an init routine in LLVM that any client has to use? I grepped llvm/include/llvm without much success.

jroelofs · November 16, 2016, 8:05pm

Hi all,

An issue that come up from time to time and has cost hours for debug for
many of us and users of LLVM is that an assert build isn’t ABI
compatible with a release build.

The CMake flags that controls this behavior is LLVM_ABI_BREAKING_CHECKS (

*LLVM_ABI_BREAKING_CHECKS*:STRING
    Used to decide if LLVM should be built with ABI breaking checks or
    not. Allowed values
    are WITH_ASSERTS (default), FORCE_ON and FORCE_OFF. WITH_ASSERTS turns
    on ABI breaking checks in an assertion enabled
    build. FORCE_ON (FORCE_OFF) turns them on (off) irrespective of
    whether normal (NDEBUG-based) assertions are enabled or not. A
    version of LLVM built with ABI breaking checks is not ABI compatible
    with a version built without it.

I propose to add a runtime check to detect when we have incorrectly
setup build.

The idea is that every translation unit that includes such header will
get a weak definition of a symbol with an initializer calling the
runtime check. The symbol name would be different if the ABI break is
enabled or not.

Can it be made into a link-time check instead? I'm imagining something like:

   #if LLVM_ENABLE_ABI_BREAKING_CHECKS
   extern int EnableABIBreakingChecks;
   __attribute__((weak)) int *VerifyEnableABIBreakingChecks = &EnableABIBreakingChecks;
   #else
   extern int DisableABIBreakingChecks;
   __attribute__((weak)) int *VerifyDisableABIBreakingChecks = &VDisableABIBreakingChecks;
   #endif

in llvm-config.h.cmake, and:

   #if LLVM_ENABLE_ABI_BREAKING_CHECKS
   int EnableABIBreakingChecks;
   #else
   int DisableABIBreakingChecks;
   #endif

in Error.cpp.

Then it'll only link if Error.cpp's TU's setting of LLVM_ENABLE_ABI_BREAKING_CHECKS matches that of the TU that includes llvm-config.h

Jon

The runtime check maintains two boolean to track if it there is in the
image at least a translation unit for each value of this flag. If both
flags are set the process is aborted.

The cost is *one* static initializer per DSO (or per image I believe).

A straw-man patch (likely not windows compatible because of weak) is:

diff --git a/llvm/include/llvm/Config/llvm-config.h.cmake
b/llvm/include/llvm/Config/llvm-config.h.cmake
index 4121e865ea00..4274c942d3b6 100644
--- a/llvm/include/llvm/Config/llvm-config.h.cmake
+++ b/llvm/include/llvm/Config/llvm-config.h.cmake
@@ -80,4 +80,18 @@
/* LLVM version string */
#define LLVM_VERSION_STRING "${PACKAGE_VERSION}"

+
+#ifdef __cplusplus
+namespace llvm {
+bool setABIBreakingChecks(bool Enabled);
+__attribute__((weak))
+#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+bool
+ABICheckEnabled = setABIBreakingChecks(true);
+#else
+bool ABICheckDisabled = setABIBreakingChecks(true);

Do you mean `false` here ^ ?

Mehdi_Amini3 · November 16, 2016, 8:12pm

Hi all,

An issue that come up from time to time and has cost hours for debug for
many of us and users of LLVM is that an assert build isn’t ABI
compatible with a release build.

The CMake flags that controls this behavior is LLVM_ABI_BREAKING_CHECKS (

LLVM_ABI_BREAKING_CHECKS:STRING
Used to decide if LLVM should be built with ABI breaking checks or
not. Allowed values
are WITH_ASSERTS (default), FORCE_ON and FORCE_OFF. WITH_ASSERTS turns
on ABI breaking checks in an assertion enabled
build. FORCE_ON (FORCE_OFF) turns them on (off) irrespective of
whether normal (NDEBUG-based) assertions are enabled or not. A
version of LLVM built with ABI breaking checks is not ABI compatible
with a version built without it.

I propose to add a runtime check to detect when we have incorrectly
setup build.

The idea is that every translation unit that includes such header will
get a weak definition of a symbol with an initializer calling the
runtime check. The symbol name would be different if the ABI break is
enabled or not.

Can it be made into a link-time check instead? I’m imagining something like:

I’d love to, but didn’t find a universal solution unfortunately

#if LLVM_ENABLE_ABI_BREAKING_CHECKS
extern int EnableABIBreakingChecks;
attribute((weak)) int *VerifyEnableABIBreakingChecks = &EnableABIBreakingChecks;
#else
extern int DisableABIBreakingChecks;
attribute((weak)) int *VerifyDisableABIBreakingChecks = &VDisableABIBreakingChecks;
#endif

in llvm-config.h.cmake, and:

#if LLVM_ENABLE_ABI_BREAKING_CHECKS
int EnableABIBreakingChecks;
#else
int DisableABIBreakingChecks;
#endif

in Error.cpp.

Then it’ll only link if Error.cpp’s TU’s setting of LLVM_ENABLE_ABI_BREAKING_CHECKS matches that of the TU that includes llvm-config.h

It seems that this work, I thought I tried exactly this but got lost on the whiteboard at some point!

Maybe because one drawback that I tried to avoid is that the export-list of a LLVM dylib would depend on the value of LLVM_ENABLE_ABI_BREAKING_CHECKS with this.

Thanks,

Mehdi_Amini3 · November 18, 2016, 11:45pm

Hi,

I had to revert it, because it breaks building LLDB on MacOS.

It turns out it would break any client that is including llvm-config.h but not linking to libLLVMSupport.
So either:

we shouldn’t allow to include llvm-config.h without linking to LLVM, in which case I need to look a bit closer as of why lldb includes this header in a context where they don’t link LLVM.
we should allow to include llvm-config.h without linking to LLVM (at least libSupport). In which case we need a new solution for this feature: it can be to use another header dedicated to this flag, that would be included only from headers that contain the ABI break.

Mehdi_Amini3 · November 19, 2016, 12:23am

The second approach is implemented here: https://reviews.llvm.org/D26876

I extracted the LLVM_ENABLE_ABI_BREAKING_CHECKS to its own header to have a fine granularity on which client needs to include the check.

Chris_Bieneman · November 20, 2016, 7:25am

Mehdi,

I think your second approach is the better option. Going with the first option means we would need to remove references to llvm-config.h in ADT, which I don’t think is a simple task.

-Chris