llvm dev - Nov 2016 - [RFC] Runtime checks for ABI breaking build of LLVM

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.

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);
+#endif
+}
+#endif
+
 #endif
diff --git a/llvm/lib/Support/Error.cpp b/llvm/lib/Support/Error.cpp
index 7436a1fd38ee..151fcdcbfb27 100644
--- a/llvm/lib/Support/Error.cpp
+++ b/llvm/lib/Support/Error.cpp
@@ -112,3 +112,17 @@ void report_fatal_error(Error Err, bool GenCrashDiag) {
 }
 
 }
+
+
+bool llvm::setABIBreakingChecks(bool Enabled) {
+  static char abi_breaking_checks_enabled = 0;
+  static char abi_breaking_checks_disabled = 0;
+  if (Enabled)
+    abi_breaking_checks_enabled = 1;
+  else
+    abi_breaking_checks_disabled = 1;
+  if (abi_breaking_checks_enabled && abi_breaking_checks_disabled)
+    report_fatal_error("Error initializing LLVM: mixing translation units
built"
+                       "with and without LLVM_ABI_BREAKING_CHECKS");
+  return true;
+}



— 
Mehdi

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20161116/acad9e06/attachment.html>

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.

On Wed, Nov 16, 2016 at 10:48 AM, Mehdi Amini via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> 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*:STRINGUsed 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.
>
> 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);
> +#endif
> +}
> +#endif
> +
>  #endif
> diff --git a/llvm/lib/Support/Error.cpp b/llvm/lib/Support/Error.cpp
> index 7436a1fd38ee..151fcdcbfb27 100644
> --- a/llvm/lib/Support/Error.cpp
> +++ b/llvm/lib/Support/Error.cpp
> @@ -112,3 +112,17 @@ void report_fatal_error(Error Err, bool GenCrashDiag)
> {
>  }
>
>  }
> +
> +
> +bool llvm::setABIBreakingChecks(bool Enabled) {
> +  static char abi_breaking_checks_enabled = 0;
> +  static char abi_breaking_checks_disabled = 0;
> +  if (Enabled)
> +    abi_breaking_checks_enabled = 1;
> +  else
> +    abi_breaking_checks_disabled = 1;
> +  if (abi_breaking_checks_enabled && abi_breaking_checks_disabled)
> +    report_fatal_error("Error initializing LLVM: mixing translation
units
> built"
> +                       "with and without
LLVM_ABI_BREAKING_CHECKS");
> +  return true;
> +}
>
>
>
> —
> Mehdi
>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20161116/0277c41a/attachment.html>

> On Nov 16, 2016, at 11:40 AM, Reid Kleckner <rnk at google.com>
wrote:
> 
> 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
<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.
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.

— 
Mehdi


> 
> On Wed, Nov 16, 2016 at 10:48 AM, Mehdi Amini via llvm-dev <llvm-dev at
lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
> 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.
> 
> 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);
> +#endif
> +}
> +#endif
> +
>  #endif
> diff --git a/llvm/lib/Support/Error.cpp b/llvm/lib/Support/Error.cpp
> index 7436a1fd38ee..151fcdcbfb27 100644
> --- a/llvm/lib/Support/Error.cpp
> +++ b/llvm/lib/Support/Error.cpp
> @@ -112,3 +112,17 @@ void report_fatal_error(Error Err, bool GenCrashDiag)
{
>  }
>  
>  }
> +
> +
> +bool llvm::setABIBreakingChecks(bool Enabled) {
> +  static char abi_breaking_checks_enabled = 0;
> +  static char abi_breaking_checks_disabled = 0;
> +  if (Enabled)
> +    abi_breaking_checks_enabled = 1;
> +  else
> +    abi_breaking_checks_disabled = 1;
> +  if (abi_breaking_checks_enabled && abi_breaking_checks_disabled)
> +    report_fatal_error("Error initializing LLVM: mixing translation
units built"
> +                       "with and without
LLVM_ABI_BREAKING_CHECKS");
> +  return true;
> +}
> 
> 
> 
> — 
> Mehdi
> 
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
> 
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20161116/c6857e74/attachment.html>

On 11/16/16 11:48 AM, Mehdi Amini via llvm-dev wrote:
> 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 ^ ?
> +#endif
> +}
> +#endif
> +
>  #endif
> diff --git a/llvm/lib/Support/Error.cpp b/llvm/lib/Support/Error.cpp
> index 7436a1fd38ee..151fcdcbfb27 100644
> --- a/llvm/lib/Support/Error.cpp
> +++ b/llvm/lib/Support/Error.cpp
> @@ -112,3 +112,17 @@ void report_fatal_error(Error Err, bool GenCrashDiag)
{
>  }
>
>  }
> +
> +
> +bool llvm::setABIBreakingChecks(bool Enabled) {
> +  static char abi_breaking_checks_enabled = 0;
> +  static char abi_breaking_checks_disabled = 0;
> +  if (Enabled)
> +    abi_breaking_checks_enabled = 1;
> +  else
> +    abi_breaking_checks_disabled = 1;
> +  if (abi_breaking_checks_enabled && abi_breaking_checks_disabled)
> +    report_fatal_error("Error initializing LLVM: mixing translation
> units built"
> +                       "with and without
LLVM_ABI_BREAKING_CHECKS");
> +  return true;
> +}
>
>
>
> —
> Mehdi
>
>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-- 
Jon Roelofs
jonathan at codesourcery.com
CodeSourcery / Mentor Embedded

> On Nov 16, 2016, at 12:05 PM, Jonathan Roelofs <jonathan at
codesourcery.com> wrote:
> 
> 
> 
> On 11/16/16 11:48 AM, Mehdi Amini via llvm-dev wrote:
>> 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




> 
> 
>> 
>> 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 ^ ?
> 
>> +#endif
>> +}
>> +#endif
>> +
>> #endif
>> diff --git a/llvm/lib/Support/Error.cpp b/llvm/lib/Support/Error.cpp
>> index 7436a1fd38ee..151fcdcbfb27 100644
>> --- a/llvm/lib/Support/Error.cpp
>> +++ b/llvm/lib/Support/Error.cpp
>> @@ -112,3 +112,17 @@ void report_fatal_error(Error Err, bool
GenCrashDiag) {
>> }
>> 
>> }
>> +
>> +
>> +bool llvm::setABIBreakingChecks(bool Enabled) {
>> +  static char abi_breaking_checks_enabled = 0;
>> +  static char abi_breaking_checks_disabled = 0;
>> +  if (Enabled)
>> +    abi_breaking_checks_enabled = 1;
>> +  else
>> +    abi_breaking_checks_disabled = 1;
>> +  if (abi_breaking_checks_enabled &&
abi_breaking_checks_disabled)
>> +    report_fatal_error("Error initializing LLVM: mixing
translation
>> units built"
>> +                       "with and without
LLVM_ABI_BREAKING_CHECKS");
>> +  return true;
>> +}
>> 
>> 
>> 
>> —
>> Mehdi
>> 
>> 
>> 
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>> 
> 
> -- 
> Jon Roelofs
> jonathan at codesourcery.com <mailto:jonathan at codesourcery.com>
> CodeSourcery / Mentor Embedded
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20161116/a2b67b67/attachment.html>