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

[Alp Toker]

On 03/01/2014 21:55, Chandler Carruth wrote:
>
> On Tue, Nov 12, 2013 at 1:20 AM, Chris Lattner <clattner at apple.com 
> <mailto:clattner at apple.com>> wrote:
>
>     n Nov 11, 2013, at 12:09 PM, Alp Toker <alp at nuanti.com
>     <mailto: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.
>
>
> 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