Goal for 3.5: Library-friendly headers

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
25 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Goal for 3.5: Library-friendly headers

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

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] Goal for 3.5: Library-friendly headers

Joerg Sonnenberger
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
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] Goal for 3.5: Library-friendly headers

Alp Toker

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
> [hidden email]         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

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

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] Goal for 3.5: Library-friendly headers

Alp Toker
In reply to this post by Joerg Sonnenberger

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
> [hidden email]         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

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

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] Goal for 3.5: Library-friendly headers

Joerg Sonnenberger
On Sun, Nov 10, 2013 at 06:07:45PM +0000, Alp Toker wrote:
> 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).

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

Joerg
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] Goal for 3.5: Library-friendly headers

Reid Kleckner-2
In reply to this post by Alp Toker
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).



On Sun, Nov 10, 2013 at 5:42 AM, Alp Toker <[hidden email]> 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
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev



_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] Goal for 3.5: Library-friendly headers

Alp Toker

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 <[hidden email]
> <mailto:[hidden email]>> 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
>     [hidden email] <mailto:[hidden email]>
>     http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>
>

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

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] Goal for 3.5: Library-friendly headers

Chris Lattner-2
In reply to this post by Alp Toker
On Nov 10, 2013, at 5:42 AM, Alp Toker <[hidden email]> 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



_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] Goal for 3.5: Library-friendly headers

NAKAMURA Takumi
In reply to this post by Alp Toker
2013/11/10 Alp Toker <[hidden email]>:
> #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.
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] Goal for 3.5: Library-friendly headers

Alp Toker
On 11/11/2013 07:37, NAKAMURA Takumi wrote:
2013/11/10 Alp Toker [hidden email]:
#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

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] Goal for 3.5: Library-friendly headers

Chris Lattner-2
On Nov 11, 2013, at 9:56 AM, Alp Toker <[hidden email]> wrote:
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.

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


_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] Goal for 3.5: Library-friendly headers

Alp Toker

On 11/11/2013 19:08, Chris Lattner wrote:

> On Nov 11, 2013, at 9:56 AM, Alp Toker <[hidden email]
> <mailto:[hidden email]>> wrote:
>> 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.
>
> 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.


>
> in a class.
>
> -Chris
>

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

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] Goal for 3.5: Library-friendly headers

Alp Toker

On 11/11/2013 19:16, Alp Toker wrote:

> On 11/11/2013 19:08, Chris Lattner wrote:
>> On Nov 11, 2013, at 9:56 AM, Alp Toker <[hidden email]
>> <mailto:[hidden email]>> wrote:
>>> 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.
>> 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.

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.


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

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] Goal for 3.5: Library-friendly headers

Reid Kleckner-2
In reply to this post by Alp Toker
On Mon, Nov 11, 2013 at 11:16 AM, Alp Toker <[hidden email]> wrote:
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.

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] Goal for 3.5: Library-friendly headers

Chris Lattner-2
In reply to this post by Alp Toker
On Nov 11, 2013, at 12:09 PM, Alp Toker <[hidden email]> wrote:

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

-Chris
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] Goal for 3.5: Library-friendly headers

Chandler Carruth-2

On Tue, Nov 12, 2013 at 1:20 AM, Chris Lattner <[hidden email]> wrote:
n Nov 11, 2013, at 12:09 PM, Alp Toker <[hidden email]> wrote:

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

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

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] Goal for 3.5: Library-friendly headers

Chris Lattner-2
On Jan 3, 2014, at 1:55 PM, Chandler Carruth <[hidden email]> wrote:
>
> 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.

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?

-Chris

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] Goal for 3.5: Library-friendly headers

Alp Toker
In reply to this post by Chandler Carruth-2

On 03/01/2014 21:55, Chandler Carruth wrote:

>
> On Tue, Nov 12, 2013 at 1:20 AM, Chris Lattner <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     n Nov 11, 2013, at 12:09 PM, Alp Toker <[hidden email]
>     <mailto:[hidden email]>> wrote:
>
>     >> 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.

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

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] Goal for 3.5: Library-friendly headers

Chandler Carruth-2
In reply to this post by Chris Lattner-2
On Fri, Jan 3, 2014 at 5:04 PM, Chris Lattner <[hidden email]> wrote:
On Jan 3, 2014, at 1:55 PM, Chandler Carruth <[hidden email]> wrote:
>
> 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.

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] Goal for 3.5: Library-friendly headers

Chandler Carruth-2
In reply to this post by Alp Toker

On Fri, Jan 3, 2014 at 5:08 PM, Alp Toker <[hidden email]> wrote:

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.

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

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
12