llvm dev - Jan 2014 - [LLVMdev] [cfe-dev] Goal for 3.5: Library-friendly headers

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

On Fri, Jan 3, 2014 at 5:08 PM, Alp Toker <alp at nuanti.com> 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).
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140103/92711de0/attachment.html>

On 03/01/2014 22:22, Chandler Carruth wrote:
>
> On Fri, Jan 3, 2014 at 5:08 PM, Alp Toker <alp at nuanti.com 
> <mailto:alp at nuanti.com>> 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?
>
We should focus on minimizing them because clang is already 100% stable, 
and LLVM can be there too with a similar effort.

Until LLVM gets there, the 3.5 llvm_config.h approach provides 
sufficient stability. In practice, clang is now already ABI stable 
because it doesn't use the variably-sized structures in its most useful 
interfaces (though I haven't announced this yet as we need some way to 
verify it).

Most of the NDEBUG-modified structures are deep in LLVM CodeGen headers 
so it's inconsequential if they ever become stabilised as far as I'm 
concerned. These are no different to source files and can as well be 
moved to lib/.

If you look at the clang changes achieving all of this, they ended up 
being a great cleanup and I've got a feeling the effort enhanced error 
recovery.

We've had a great push for enhanced diagnostics in the last couple of 
months, and just throwing in an assert or adding a NDEBUG struct field 
is no longer accepted in polite clang circles. This goal is part of that 
story :-)
> 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).
I'm not OK with adding new NDEBUGs to any headers. Any NDEBUG is 
guaranteed to yield the wrong result 50% of the time when included in an 
external project, so it's never valid to add a new one. If you need 
this, there's almost certainly a better way to achieve it and you can 
ask on the list for suggestions.

API / ABI stability aside, NDEBUG definitions headers have done little 
good and a lot of harm in getting LLVM usable as a library. We're 
missing out as long as we make embedding difficult and this has broken 
for most of LLVM's lifetime -- it's only now that we're now starting
to
get results and some degree of stability on ToT and this is something to 
be excited about.

Alp.


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