llvm dev - Nov 2013 - [LLVMdev] Goal for 3.5: Library-friendly headers

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.

-- 
http://www.nuanti.com
the browser experts

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

On Sun, Nov 10, 2013 at 01:42:24PM +0000, Alp Toker
wrote:
> |#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.

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

On 10/11/2013 14:26, Joerg Sonnenberger wrote:
> On Sun, Nov 10, 2013 at 01:42:24PM +0000, Alp Toker wrote:
>> |#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 ;-)

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.

>
> 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
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
-- 
http://www.nuanti.com
the browser experts

On 10/11/2013 14:26, Joerg Sonnenberger wrote:
> On Sun, Nov 10, 2013 at 01:42:24PM +0000, Alp Toker wrote:
>> |#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.
>
> 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
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
-- 
http://www.nuanti.com
the browser experts

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


On Sun, Nov 10, 2013 at 5:42 AM, Alp Toker <alp at nuanti.com> wrote:
>  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.
>
> -- http://www.nuanti.com
> the browser experts
>
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20131110/39f4e65b/attachment.html>

On 11/11/2013 04:51, Reid Kleckner wrote:
> 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.

>
> 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
>
>
> On Sun, Nov 10, 2013 at 5:42 AM, Alp Toker <alp at nuanti.com
> <mailto:alp at nuanti.com>> wrote:
>
>     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.
>
>     -- 
>     http://www.nuanti.com
>     the browser experts
>
>
>     _______________________________________________
>     cfe-dev mailing list
>     cfe-dev at cs.uiuc.edu <mailto:cfe-dev at cs.uiuc.edu>
>     http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>
>
-- 
http://www.nuanti.com
the browser experts

On Nov 10, 2013, at 5:42 AM, Alp Toker <alp at nuanti.com>
wrote:
> 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


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

2013/11/10 Alp Toker <alp at nuanti.com>:
> #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!
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.

On 11/11/2013 07:37, NAKAMURA Takumi wrote:
> 2013/11/10 Alp Toker <alp at nuanti.com>:
>> #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!
> 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.
Done :-)

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.

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:

|This patchset against LLVM r194356 implements API stability.||
||
||Embedding is now fully independent of build configuration, with the
exception of C++11 feature checks in Compiler.h which still need to be
autoconf'ed.||
||
||External applications should include llvm-config.h. ||
||
||Only supported with the CMake build system, Makefile is TBD.|

Alp.

-- 
http://www.nuanti.com
the browser experts

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