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

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

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 <alp at nuanti.com
>> <mailto:alp at nuanti.com>> 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

On Mon, Nov 11, 2013 at 11:16 AM, Alp Toker <alp at nuanti.com>
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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20131111/64ee7ed1/attachment.html>

On Nov 11, 2013, at 12:09 PM, Alp Toker <alp at nuanti.com> 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