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

If this information is useful, please help other people find it:
Share via:

Mehdi Amini via llvm-dev

2016-Nov-16 20:12 UTC

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

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

Mehdi Amini via llvm-dev

2016-Nov-18 23:45 UTC

head link

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

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


> On Nov 16, 2016, at 12:12 PM, Mehdi Amini via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
>> 
>> On Nov 16, 2016, at 12:05 PM, Jonathan Roelofs <jonathan at
codesourcery.com <mailto: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 <mailto: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
> 
> _______________________________________________
> 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/20161118/f2ee11c8/attachment.html>

Mehdi Amini via llvm-dev

2016-Nov-19 00:23 UTC

head link

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

> On Nov 18, 2016, at 3:45 PM, Mehdi Amini <mehdi.amini at apple.com>
wrote:
> 
> 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.
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.

— 
Mehdi



> 
> 
>> On Nov 16, 2016, at 12:12 PM, Mehdi Amini via llvm-dev <llvm-dev at
lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
>> 
>>> 
>>> On Nov 16, 2016, at 12:05 PM, Jonathan Roelofs <jonathan at
codesourcery.com <mailto: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 <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>
>>>> 
>>> 
>>> -- 
>>> Jon Roelofs
>>> jonathan at codesourcery.com <mailto:jonathan at
codesourcery.com>
>>> CodeSourcery / Mentor Embedded
>> 
>> _______________________________________________
>> 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/20161118/c4fb84e0/attachment-0001.html>

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

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

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

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