Goal for 3.5: Library-friendly headers

Project Infrastructure LLVM Dev List Archives
#1

With the recent thread on using C++11 in LLVM/clang, one of the recurring themes was a desire to make the internal headers more consumable.

While I don’t personally want any kind of stability guarantee for internal headers, I do think we can do more to ensure that any given snapshot of the headers is consumable from different compilers and build configurations.

In practice this means removing certain preprocessor conditionals under include/ and eventually introducing a config.h in order to make structures stable across different configurations.

So here’s a concrete proposal…

The most common violations I can see at present are in llvm/Support, so if we skip over it for now (as a candidate for making private), what remains is actually fairly realistic to clean up…

#ifndef NDEBUG

This is the biggest violation. NDEBUG should only ever be used in source files. That way if something is crashing we can swap in a debug build without rebuilding every single dependent application. Win!

undefs

Some examples from llvm/include and clang/include:

#undef NetBSD
#undef mips
#undef sparc
#undef INT64_MAX
#undef alloca

This is not OK to do globally – even if LLVM doesn’t care about the definition, maybe embedding applications or OS headers do?

$ find include -iname '*.h' | xargs grep '^#' -- | grep -vE '^.*:.*(LLVM_|cplusplus|endif|else|include|define )'| grep -vw Support | wc -l
```57`

I think it’s perfectly realistic to get that number down to 0.

We can start by putting a check like that in the build system, first to emit a warning, and ultimately to make violations a build error.

This is something we can start working towards today. Any objections?

Alp.

#2

Given that 3 of the 5 #undefs are directly relevant for me -- they exist
because OS headers and/or compiler default configurations provide legacy
macros that can't easily be dropped without breaking old cruft. All
modernish software is supposed to use the corresponding __ version of
the names.

INT64_MAX as seen by the context is about a AIX specific header bug.

alloca has a comment about VC++. Might be better to add a conditional
around it and see if that solves the problem.

In short, I don't see a real problem here.

Joerg

#3

>#undef NetBSD||
>>#undef mips||
>>#undef sparc||
>>#undef INT64_MAX||
>>#undef alloca|

This is not OK to do globally -- even if LLVM doesn't care about the
definition, maybe embedding applications or OS headers do?

Given that 3 of the 5 #undefs are directly relevant for me -- they exist
because OS headers and/or compiler default configurations provide legacy
macros that can't easily be dropped without breaking old cruft. All
modernish software is supposed to use the corresponding __ version of
the names.

Library design means thinking about these things:

1) Not using compiler-predefined macro names as identifiers in headers:
If it's known for a fact that GCC defines some name on a platform we
care about, it's likely that some OS or application header somewhere
depends on it. Undefining, especially out of order as in Triple.h, isn't
a solution. If you know about these three, then great, we already found
someone who can implement the correct fix :wink:

2) Not making declarations conditional on NDEBUG or other
configuration-specific defines, including compiler features: Right now,
if you have a release build of LLVM and using the C++ interface, you
have to build your own application with NDEBUG.

These should certainly interest people who've tried embedding LLVM in
non-trivial external applications.

The subtext here, of course, is that this may even go some way to
providing access from non-C++11 compilers for some amount of time after
the first C++11 features start to land in the 3.5 series.

It'll be interesting to see who's willing step up and help here with
patches, as opposed to just being in the "me too" crowd who post to the
list.

Alp.

#4

>#undef NetBSD||
>>#undef mips||
>>#undef sparc||
>>#undef INT64_MAX||
>>#undef alloca|

This is not OK to do globally -- even if LLVM doesn't care about the
definition, maybe embedding applications or OS headers do?

Given that 3 of the 5 #undefs are directly relevant for me -- they exist
because OS headers and/or compiler default configurations provide legacy
macros that can't easily be dropped without breaking old cruft. All
modernish software is supposed to use the corresponding __ version of
the names.

As an extra data point, clang itself pre-defines PSP, qdsp6, hexagon and
MSP430, and it looks from a web search that platform compilers have a
tradition of defining the platform and architecture like this so the
only reason #undef NetBSD is alone there is that you were the first to
compile on the less common platforms who actually bothered to submit a
patch.

The right fix would be to prefix the enumerands in Triple.h or move the
enums into a private header (probably not feasible because they're used
externally).

Alp.

#5

The former would make the code more ugly for no real gain and the latter
won't fix the "problem".

Joerg

#6

I don’t think NDEBUG is that easy. We use assert liberally[1] in templated code in headers. What about cast<>, which has assert(isa<…>…)? On ELF, you have an ODR problem, and you’ll get either one or the other based on the whims of the dynamic linker.

You could probably get things to the point that it sort of works, but I don’t think we could support it without testing. We could test linking Release clang to Debug LLVM (heck, that probably works, ODR problems aside).

[1] http://llvm.org/docs/CodingStandards.html#assert-liberally

#7

I don't think NDEBUG is that easy. We use assert liberally[1] in
templated code in headers. What about cast<>, which has
assert(isa<...>...)? On ELF, you have an ODR problem, and you'll get
either one or the other based on the whims of the dynamic linker.

I have llvm_assert() working already. The patchset is large but split
out and reproducible.

The majority of checks change to llvm_assert(), and the ones that depend
on NDEBUG change to llvm_debug_assert(). Together they solve ODR neatly.

This also fixes the long-standing kludge of undefining/redefining NDEBUG
in the build system, lets you build genuine Release+Asserts, fixes the u
n r e a d a b l e UTF-16 MSVC assert() output and breaks the awful debug
C++ MSVCRT dependency on the on Windows. NDEBUG had a lot of baggage
that we don't really want.

Best of all, you get beta release builds with asserts enabled to provide
useful feedback during the development cycle that are still reasonably
optimised.

(I've got a feeling this could also turn out to be an easy way to
provide symbolisation based on asserts in an unevaluated context the
same way it's already done with llvm_unreachable(). The information is
lost in Release with the old setup.)

So, the patchset is a bit large for the list. Any suggestions where to
post it?

Alp.

#8

With the recent thread on using C++11 in LLVM/clang, one of the recurring themes was a desire to make the internal headers more consumable.

Great! I strongly agree with your goals, though have a few questions below:

#ifndef NDEBUG

This is the biggest violation. NDEBUG should only ever be used in source files. That way if something is crashing we can swap in a debug build without rebuilding every single dependent application. Win!

+1

undefs

Some examples from llvm/include and clang/include:

#undef INT64_MAX

INT64_MAX is specific to AIX. I’m not sure that we even build on AIX anymore, but in any case, the people who care about AIX can figure out what they want to do.

#undef alloca

This looks like a problem in clang/include/Basic/Builtins.h. I agree it should be fixed.

#undef NetBSD
#undef mips
#undef sparc

These (and the other ones in llvm/Support/Solaris.h) are fine and we should keep them. These exist because of system headers that trample on the global namespace. Each of these is also defined with an __ version as well, and are only preserved for legacy reasons.

The only impact of LLVM containing these is that code cannot use the legacy names and include an LLVM header. It seems perfectly reasonable for me to require code to use the __ version of a name if it is updating to use LLVM headers. Adding new features to legacy code makes that code non-legacy in my books.

-Chris

#9

I wish;

  - NDEBUG may not modify API. class structure (member offset, vtables)
    should be stable and identical among Release builds and Debug builds.
  - NDEBUG may keep and add facilities.

#10

Done :slight_smile:

The patchset is 532K so I’ve put it online:

The bulk edits are split out and noted. They were refactored with an internal tool, so it’s not a big hassle to keep this up to date until 3.4 is out the door. A handful of fixes were needed to add support for Release+Assert builds and these are also separate commits. TableGen and internal tools continue to use ordinary platform assert() / NDEBUG (I think a few uses might have been incorrectly changed, will check over), as do the C++ sources and non-public headers. llvm_assert() is a bit more verbose but it actually fits into the coding style well, and it’s grown on me. Extract from README.txt: Alp.

#11

Whoa whoa whoa. Why are you introducing an llvm_assert() macro? The use of assert in header files is not a problem for “libraries”, it is things like:

#ifndef NDEBUG
int SomeNewVariable;
#endif

in a class.

-Chris

#12

Done :slight_smile:

The patchset is 532K so I've put it online:

  http://www.nuanti.com/tmp/llvm-api-stability/

The bulk edits are split out and noted. They were refactored with an
internal tool, so it's not a big hassle to keep this up to date until
3.4 is out the door.

A handful of fixes were needed to add support for Release+Assert
builds and these are also separate commits.

Whoa whoa whoa. Why are you introducing an llvm_assert() macro? The
use of assert in header files is not a problem for "libraries", it is
things like:

#ifndef NDEBUG
  int SomeNewVariable;
#endif

They're both are a problem. assert() is defined deep down in the C
library headers and is conditional on !NDEBUG. It's not practical to
override the preprocessor define, at least with the MSVC and OS X
headers. (It _might_ be possible to hack around with glibc headers but
I'm not certain.)

That means that, as long as there are assert() uses in the headers and
you want to have a standard definition usable from a non-debug build,
you need a custom assert() macro.

Even when you have a !NDEBUG build, the platform assert() is pretty
crummy on Windows and generates, at best a UTF-16 dump, or sometimes
just pops up a dialog. WebKit and other projects take the same approach
and define their own assertion macros to deal with this portably.

So as far as I can tell, as long as the headers use assert(), they need
to use our own version in order for the definition to match.

Alp.

#13

Chris,

Having said that, I agree it's worth trying without llvm_assert() to see
how far it gets.

I'll pull together a rework of this patchset with just the build system
and structure changes alone to see how far it gets.

The key thing then is to make sure that it's safe to enable the
assertions in the headers if an application is built with !NDEBUG and
linked against an NDEBUG version of LLVM.

Alp.

#14

Even when you have a !NDEBUG build, the platform assert() is pretty
crummy on Windows and generates, at best a UTF-16 dump, or sometimes
just pops up a dialog.

Agreed. We have lots of hacks surrounding this. See
LLVM_DISABLE_CRASH_REPORTING in lib/Support/Windows/Signals.inc. I've also
had to change ninja to pass through UTF-16 output from Clang assertion
failures.

Having our own error reporting would be nice for Windows, but it would take
us further away from standard C library features. We're already using
llvm_unreachable everywhere, so this might not be so bad.

WebKit and other projects take the same approach

and define their own assertion macros to deal with this portably.

So as far as I can tell, as long as the headers use assert(), they need
to use our own version in order for the definition to match.

Even though I brought it up, the ODR problem feels small enough to skate by
without worrying about it. The typical use case for embedders is to turn
off NDEBUG so they can debug, which means LLVM's cast<> implementation
might get slow if the linker picks the wrong definition to call from LLVM
libs.

#15

Sounds great. I'm pretty confident that there will be no problems - in practice - from any ODR violations that might arise from "assert" differing across library boundaries. We would want some pretty strong practical justification for breaking away from standard assert.

-Chris

#16

Sorry to dig up this thread, but when re-reading it, I was surprised that
everyone seems to think this will be easily done across all of LLVM.

How can we support AssertingVH, which behaves as a POD-like struct around a
pointer in NDEBUG, and as a class with significant (important)
functionality to implement asserts on dangling value handles in !NDEBUG
builds?

While having different components of LLVM and consumers of LLVM able to
intermix NDEBUG and !NDEBUG built code freely without ABI issues is
nice-to-have in my book, the functionality provided by AssertingVH is
significantly more nice-to-have, and I don't see any easy ways to contain
or limit the exposure of this facility to library-level consumers.

We also have quite a few places in LLVM today that conserve memory usage in
non-assert builds by removing unnecessary members of classes. We can say
that this makes the ABI more fragile, but it is a valuable space
optimization. Chris, are you saying to strongly believe that these should
only be allowed for classes that are not visible in the C++ API? I find
that surprising, as LLVM has historically prioritized efficiency and
developer tools over ABI stability in our C++ APIs. Build-level instability
is certainly a different beast from version stability, but I wanted to make
sure the ramifications of this shift were being considered by everyone.
(They hadn't by me.)

#17

I hadn’t considered AssertVH, and I agree that losing it isn’t an option.

Would it be possible to redesign AssertVH to be non-ABI fragile across debug/release builds? I haven’t looked at it recently, but maybe it could be a pointer to a CallbackVH in the debug mode, or a PointerUnion<rawpointer, callbackvh> or something.

Do you have any specific examples in mind?

-Chris

#18

    n Nov 11, 2013, at 12:09 PM, Alp Toker <alp@nuanti.com

    >> Even when you have a !NDEBUG build, the platform assert() is pretty
    >> crummy on Windows and generates, at best a UTF-16 dump, or
    sometimes
    >> just pops up a dialog. WebKit and other projects take the same
    approach
    >> and define their own assertion macros to deal with this portably.
    >>
    >> So as far as I can tell, as long as the headers use assert(),
    they need
    >> to use our own version in order for the definition to match.
    >
    > Chris,
    >
    > Having said that, I agree it's worth trying without
    llvm_assert() to see
    > how far it gets.
    >
    > I'll pull together a rework of this patchset with just the build
    system
    > and structure changes alone to see how far it gets.
    >
    > The key thing then is to make sure that it's safe to enable the
    > assertions in the headers if an application is built with
    !NDEBUG and
    > linked against an NDEBUG version of LLVM.

    Sounds great. I'm pretty confident that there will be no problems
    - in practice - from any ODR violations that might arise from
    "assert" differing across library boundaries. We would want some
    pretty strong practical justification for breaking away from
    standard assert.

Sorry to dig up this thread, but when re-reading it, I was surprised that everyone seems to think this will be easily done across all of LLVM.

It is almost done. It was far easier than the original patch.

How can we support AssertingVH, which behaves as a POD-like struct around a pointer in NDEBUG, and as a class with significant (important) functionality to implement asserts on dangling value handles in !NDEBUG builds?

This was in the original patch. If you have a moment I recommend that you take a look, particularly at the config.h change which is still the current 3.5 plan (the llvm_assert changes which make the bulk of the patch aren't in the plan for now though).

To summarise, it's a matter of replacing the 5 or so structurally significant NDEBUG with a LLVM_DEBUG_BUILD macro definition in the generated llvm-config.h header.

While having different components of LLVM and consumers of LLVM able to intermix NDEBUG and !NDEBUG built code freely without ABI issues is nice-to-have in my book, the functionality provided by AssertingVH is significantly more nice-to-have, and I don't see any easy ways to contain or limit the exposure of this facility to library-level consumers.

As explained above, we will retain AssertingVH exactly as is, with the structure adapting exactly as it does now to include extra bits and asserts between DEBUG and NDEBUG.

The only difference is that the macro definition will be in llvm-debug.h so it'll be stable independent of external project configurations.

We also have quite a few places in LLVM today that conserve memory usage in non-assert builds by removing unnecessary members of classes. We can say that this makes the ABI more fragile, but it is a valuable space optimization.

These can also use the above scheme to achieve embedding stability.

In practice though in the clang headers we found that most of the fields were harmless, adding no more than one pointer's worth to a process singleton class. Which is really not even a drop in the ocean.

Chris, are you saying to strongly believe that these should only be allowed for classes that are not visible in the C++ API? I find that surprising, as LLVM has historically prioritized efficiency and developer tools over ABI stability in our C++ APIs. Build-level instability is certainly a different beast from version stability, but I wanted to make sure the ramifications of this shift were being considered by everyone. (They hadn't by me.)

I think you misunderstood the approach. Obviously I'm not going to propose something that drastically changes the way structures are defined right now.

The early patch, despite it's bulk, covers all of this and is a good place to check if you want to see the exact implementation approach..

I also recommend that you check the commits that got clang NDEBUG-free in headers.

They're both pretty neat patchsets and it's amazing we got the results we wanted with such a small delta.

Alp.

#19

>

> The key thing then is to make sure that it's safe to enable the
> assertions in the headers if an application is built with !NDEBUG and
> linked against an NDEBUG version of LLVM.

Sounds great. I'm pretty confident that there will be no problems - in
practice - from any ODR violations that might arise from "assert" differing
across library boundaries. We would want some pretty strong practical
justification for breaking away from standard assert.

Sorry to dig up this thread, but when re-reading it, I was surprised that
everyone seems to think this will be easily done across all of LLVM.

How can we support AssertingVH, which behaves as a POD-like struct around
a pointer in NDEBUG, and as a class with significant (important)
functionality to implement asserts on dangling value handles in !NDEBUG
builds?

While having different components of LLVM and consumers of LLVM able to
intermix NDEBUG and !NDEBUG built code freely without ABI issues is
nice-to-have in my book, the functionality provided by AssertingVH is
significantly more nice-to-have, and I don't see any easy ways to contain
or limit the exposure of this facility to library-level consumers.

I hadn’t considered AssertVH, and I agree that losing it isn’t an option.

Would it be possible to redesign AssertVH to be non-ABI fragile across
debug/release builds? I haven’t looked at it recently, but maybe it could
be a pointer to a CallbackVH in the debug mode, or a
PointerUnion<rawpointer, callbackvh> or something.

If you want methods to still be inlined in the non-assert case, you still
have an ABI break in how you interpret the pointer...

We also have quite a few places in LLVM today that conserve memory usage
in non-assert builds by removing unnecessary members of classes. We can say
that this makes the ABI more fragile, but it is a valuable space
optimization. Chris, are you saying to strongly believe that these should
only be allowed for classes that are not visible in the C++ API? I find
that surprising, as LLVM has historically prioritized efficiency and
developer tools over ABI stability in our C++ APIs. Build-level instability
is certainly a different beast from version stability, but I wanted to make
sure the ramifications of this shift were being considered by everyone.
(They hadn't by me.)

Do you have any specific examples in mind?

Several passes do this, but currently they are hidden in a single TU. It
would only become a problem if/when they had logic hoisted to an analysis.
The best current example I found with a quick search is
FunctionLoweringInfo, but maybe we could pay the cost there.

#20

If we're going to have structural differences between an asserts and a
no-asserts build, why should we stress about minimizing them? That is, why
not just s/NDEBUG/LLVM_NDEBUG/g (conceptually, not textually of course) and
provide a per-build stable idea of whether "asserts" are on or off?

That would seem way simpler, and would allow library users to define NDEBUG
which ever way they wanted without any impact on LLVM. It would also allow
us to not think about whether or not the uses of NDEBUG that are left in
the headers are "safe" ODR violations (something I'm pretty suspect about
to begin with in an LTO-enabled world).