llvm dev - Aug 2015 - [LLVMdev] Ideas for making llvm-config --cxxflags more useful

> On Aug 7, 2015, at 12:32 PM, David Blaikie <dblaikie at gmail.com>
wrote:
> 
> 
> 
> On Fri, Aug 7, 2015 at 11:30 AM, Pete Cooper <peter_cooper at apple.com
<mailto:peter_cooper at apple.com>> wrote:
> I’ve almost finished a patch to add back in either out of line destructors
or anchor methods.  We seem to use one or the one, relatively inconsistently.
> 
> What i’ve gone for is that if a class already had an inline destructor then
i left it alone and added an anchor method.  Otherwise I added an out of line
destructor.
> 
> That seems possibly the opposite of what might be desired - if the class
has no user-declared dtor, it's implicitly inline and possibly trivial
(& doesn't suppress implicit move and copy ops (rule of 0/5)) - I'd
avoid adding a dtor to a class that doesn't have/need one (& whenever I
see empty-bodied dtors I try to remove them - not always possible).
> 
> I was probably the first one to use (or at least do a lot of them) the
explicit anchors in an attempt to make LLVM -Wweak-vtables clean but never got
around to going all the way there. I think Chris Lattner even advocated for not
out-of-lining dtors just to make the vtables strong, instead using an explicit
anchor. But I'm pretty ambivalent about it. In theory the style guide
advocates this for build perf reasons - which would be interesting to test &
see if it's actually relevant. If it's a matter of
correctness/functionality as this thread seems to imply (but I haven't read
it enough to really understand what's being dealt with here, to be honest)
then I guess we'll have to decide on some approach... *shrug*
Thanks for the explanation.  I knew we wouldn’t be able to inline the
destructors when they are out of line, but i didn’t know about the rule of 0/5
which is also interesting.

So I didn’t really like the look of what I did either which is why i thought it
best to explain it.  I’m happy to change over what I’ve done to anchor’s, but
its unfortunate that it will then differ from some of the other sibling classes.

Perhaps this is moot* though, if I can get back to finishing the
devirtualisation without any perf issues.

* moot for ToT perhaps.  I’m happy to get something for 3.7 if we can work out
what it should be.  The options seem to be
- Do nothing to the code, except perhaps change llvm-config as David said
- This patch
- The equivalent to this with anchor’s.

Anyone got a strong opinion which is best, especially as I don’t want to do
anything too big for 3.7 at this point.

Cheers,
Pete
> 
> - Dave
>  
> 
> Now if I compile Instructions.cpp with -Wweak-vtable, the only warnings
given are:
> 
> ../include/llvm/PassSupport.h:226:8: warning:
'PassRegistrationListener' has no out-of-line virtual method
definitions;
>       its vtable will be emitted in every translation unit [-Wweak-vtables]
> struct PassRegistrationListener {
>        ^
> In file included from ../lib/IR/Instructions.cpp:16:
> In file included from ../lib/IR/LLVMContextImpl.h:19:
> In file included from ../lib/IR/ConstantsContext.h:25:
> ../include/llvm/Support/raw_ostream.h:321:7: warning:
'raw_pwrite_stream' has no out-of-line virtual method
>       definitions; its vtable will be emitted in every translation unit
[-Wweak-vtables]
> class raw_pwrite_stream : public raw_ostream {
>       ^
> ../include/llvm/Support/raw_ostream.h:539:7: warning:
'buffer_ostream' has no out-of-line virtual method definitions;
>       its vtable will be emitted in every translation unit [-Wweak-vtables]
> class buffer_ostream : public raw_svector_ostream {
> 
> However, I do wonder how useful my patch for Instructions.h (and
constants.h too which needed work), when you can’t #include PassSupport without
-fno-rtti.
> 
> So, i’l get a patch out soon to look at, but i would like to discuss
whether any of you think its worth it given the other warnings which still
prevent no-rtti.
> 
> Cheers,
> Pete
> 
>> On Aug 7, 2015, at 10:54 AM, Pete Cooper via llvm-dev <llvm-dev at
lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
>> 
>> Sorry about this. I think if I'd finished the work to remove
vtables from Value then it wouldn't be an issue, but I put that on hold due
to performance concerns.
>> 
>> I can add back in a bunch of anchor functions where needed. Will just
need to go through and find all the classes which need them.
>> 
>> Pete
>> 
>> Sent from my iPhone
>> 
>> On Aug 7, 2015, at 10:32 AM, David Blaikie <dblaikie at gmail.com
<mailto:dblaikie at gmail.com>> wrote:
>> 
>>> 
>>> 
>>> On Fri, Aug 7, 2015 at 10:22 AM, Hans Wennborg via llvm-dev
<llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>>
wrote:
>>> On Thu, Aug 6, 2015 at 12:04 PM, David Chisnall via llvm-dev
>>> <llvm-dev at lists.llvm.org <mailto:llvm-dev at
lists.llvm.org>> wrote:
>>> > [Ooops, sent to the old list address by mistake]
>>> >
>>> > On 30 Jul 2015, at 21:04, tom at stellard.net <mailto:tom
at stellard.net> wrote:
>>> >>
>>> >> For flags like -fno-rtti (are there others?) that are
required in some cases
>>> >> (I think -fno-rtti is required only if you sub-class LLVM
objects), I would propose
>>> >> adding a separate flag like --uses-rtti.  This would give
users more fine-grained
>>> >> control over which flags they use, and also would let them
choose the correct
>>> >> flag since, for example, -fno-rtti is not understood by
MSVC.
>>> >
>>> > There appears to be a regression in LLVM 3.7, which means that
you must compile with -fno-rtti if you include llvm’s Instructions.h.  The issue
is that a few of the classes (ICmpInst, GetElementPtrInst and PHINode) are now
defined entirely in the header, so every compilation unit that includes the
header will emit them.  These classes all inherit from Instruction (indirectly
via CmpInst in the case of ICmpInst) and so fail to link if compiled with
-fno-rtti, because they can’t find the vtable for ICmpInst.
>>> 
>>> I looked at the file, and this didn't seem true (e.g.
>>> GetElementPtrInst::init is still out-of-line). But then I realized
you
>>> mean virtual functions, so these classes no longer have a key
>>> function.
>>> 
>>> This is probably Pete's r240588. I suppose we could add key
functions
>>> to these classes (even if they're not used for anything).
I'm not sure
>>> how we'd prevent this from regressing though :-/
>>> 
>>> In theory the LLVM style guide mandates key functions for all
dynamic classes (under the claim of build performance - avoiding duplicate
vtable emission, etc). We've never strongly enforced it though - if we
really wanted to, we could do so as Clang has a warning that triggers whenever a
vtable is emitted weakly (which is what happens when there isn't a key
function).
>>> 
>>> - David
>>>  
>>> 
>>>  - Hans
>>> _______________________________________________
>>> LLVM Developers mailing list
>>> llvm-dev at lists.llvm.org <mailto:llvm-dev at
lists.llvm.org>         http://llvm.cs.uiuc.edu
<http://llvm.cs.uiuc.edu/>
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
>>> 
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>   
http://llvm.cs.uiuc.edu <http://llvm.cs.uiuc.edu/>
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
> 
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150807/a9fd7243/attachment.html>

On Fri, Aug 07, 2015 at 02:13:14PM -0700, Pete Cooper via llvm-dev
wrote:
> 
> > On Aug 7, 2015, at 12:32 PM, David Blaikie <dblaikie at
gmail.com> wrote:
> > 
> > 
> > 
> > On Fri, Aug 7, 2015 at 11:30 AM, Pete Cooper <peter_cooper at
apple.com <mailto:peter_cooper at apple.com>> wrote:
> > I’ve almost finished a patch to add back in either out of line
destructors or anchor methods.  We seem to use one or the one, relatively
inconsistently.
> > 
> > What i’ve gone for is that if a class already had an inline destructor
then i left it alone and added an anchor method.  Otherwise I added an out of
line destructor.
> > 
> > That seems possibly the opposite of what might be desired - if the
class has no user-declared dtor, it's implicitly inline and possibly trivial
(& doesn't suppress implicit move and copy ops (rule of 0/5)) - I'd
avoid adding a dtor to a class that doesn't have/need one (& whenever I
see empty-bodied dtors I try to remove them - not always possible).
> > 
> > I was probably the first one to use (or at least do a lot of them) the
explicit anchors in an attempt to make LLVM -Wweak-vtables clean but never got
around to going all the way there. I think Chris Lattner even advocated for not
out-of-lining dtors just to make the vtables strong, instead using an explicit
anchor. But I'm pretty ambivalent about it. In theory the style guide
advocates this for build perf reasons - which would be interesting to test &
see if it's actually relevant. If it's a matter of
correctness/functionality as this thread seems to imply (but I haven't read
it enough to really understand what's being dealt with here, to be honest)
then I guess we'll have to decide on some approach... *shrug*
> Thanks for the explanation.  I knew we wouldn’t be able to inline the
destructors when they are out of line, but i didn’t know about the rule of 0/5
which is also interesting.
> 
> So I didn’t really like the look of what I did either which is why i
thought it best to explain it.  I’m happy to change over what I’ve done to
anchor’s, but its unfortunate that it will then differ from some of the other
sibling classes.
> 
> Perhaps this is moot* though, if I can get back to finishing the
devirtualisation without any perf issues.
> 
> * moot for ToT perhaps.  I’m happy to get something for 3.7 if we can work
out what it should be.  The options seem to be
> - Do nothing to the code, except perhaps change llvm-config as David said
> - This patch
> - The equivalent to this with anchor’s.
> 
> Anyone got a strong opinion which is best, especially as I don’t want to do
anything too big for 3.7 at this point.
> 
I don't have any strong opinions.  Did this actually break anyone's use
case or is it just a hypothetical problem?  It doesn't seem like there
is much value in fixing this until llvm-config is also fixed.

-Tom
> Cheers,
> Pete
> > 
> > - Dave
> >  
> > 
> > Now if I compile Instructions.cpp with -Wweak-vtable, the only
warnings given are:
> > 
> > ../include/llvm/PassSupport.h:226:8: warning:
'PassRegistrationListener' has no out-of-line virtual method
definitions;
> >       its vtable will be emitted in every translation unit
[-Wweak-vtables]
> > struct PassRegistrationListener {
> >        ^
> > In file included from ../lib/IR/Instructions.cpp:16:
> > In file included from ../lib/IR/LLVMContextImpl.h:19:
> > In file included from ../lib/IR/ConstantsContext.h:25:
> > ../include/llvm/Support/raw_ostream.h:321:7: warning:
'raw_pwrite_stream' has no out-of-line virtual method
> >       definitions; its vtable will be emitted in every translation
unit [-Wweak-vtables]
> > class raw_pwrite_stream : public raw_ostream {
> >       ^
> > ../include/llvm/Support/raw_ostream.h:539:7: warning:
'buffer_ostream' has no out-of-line virtual method definitions;
> >       its vtable will be emitted in every translation unit
[-Wweak-vtables]
> > class buffer_ostream : public raw_svector_ostream {
> > 
> > However, I do wonder how useful my patch for Instructions.h (and
constants.h too which needed work), when you can’t #include PassSupport without
-fno-rtti.
> > 
> > So, i’l get a patch out soon to look at, but i would like to discuss
whether any of you think its worth it given the other warnings which still
prevent no-rtti.
> > 
> > Cheers,
> > Pete
> > 
> >> On Aug 7, 2015, at 10:54 AM, Pete Cooper via llvm-dev <llvm-dev
at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
> >> 
> >> Sorry about this. I think if I'd finished the work to remove
vtables from Value then it wouldn't be an issue, but I put that on hold due
to performance concerns.
> >> 
> >> I can add back in a bunch of anchor functions where needed. Will
just need to go through and find all the classes which need them.
> >> 
> >> Pete
> >> 
> >> Sent from my iPhone
> >> 
> >> On Aug 7, 2015, at 10:32 AM, David Blaikie <dblaikie at
gmail.com <mailto:dblaikie at gmail.com>> wrote:
> >> 
> >>> 
> >>> 
> >>> On Fri, Aug 7, 2015 at 10:22 AM, Hans Wennborg via llvm-dev
<llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>>
wrote:
> >>> On Thu, Aug 6, 2015 at 12:04 PM, David Chisnall via llvm-dev
> >>> <llvm-dev at lists.llvm.org <mailto:llvm-dev at
lists.llvm.org>> wrote:
> >>> > [Ooops, sent to the old list address by mistake]
> >>> >
> >>> > On 30 Jul 2015, at 21:04, tom at stellard.net
<mailto:tom at stellard.net> wrote:
> >>> >>
> >>> >> For flags like -fno-rtti (are there others?) that are
required in some cases
> >>> >> (I think -fno-rtti is required only if you sub-class
LLVM objects), I would propose
> >>> >> adding a separate flag like --uses-rtti.  This would
give users more fine-grained
> >>> >> control over which flags they use, and also would let
them choose the correct
> >>> >> flag since, for example, -fno-rtti is not understood
by MSVC.
> >>> >
> >>> > There appears to be a regression in LLVM 3.7, which means
that you must compile with -fno-rtti if you include llvm’s Instructions.h.  The
issue is that a few of the classes (ICmpInst, GetElementPtrInst and PHINode) are
now defined entirely in the header, so every compilation unit that includes the
header will emit them.  These classes all inherit from Instruction (indirectly
via CmpInst in the case of ICmpInst) and so fail to link if compiled with
-fno-rtti, because they can’t find the vtable for ICmpInst.
> >>> 
> >>> I looked at the file, and this didn't seem true (e.g.
> >>> GetElementPtrInst::init is still out-of-line). But then I
realized you
> >>> mean virtual functions, so these classes no longer have a key
> >>> function.
> >>> 
> >>> This is probably Pete's r240588. I suppose we could add
key functions
> >>> to these classes (even if they're not used for anything).
I'm not sure
> >>> how we'd prevent this from regressing though :-/
> >>> 
> >>> In theory the LLVM style guide mandates key functions for all
dynamic classes (under the claim of build performance - avoiding duplicate
vtable emission, etc). We've never strongly enforced it though - if we
really wanted to, we could do so as Clang has a warning that triggers whenever a
vtable is emitted weakly (which is what happens when there isn't a key
function).
> >>> 
> >>> - David
> >>>  
> >>> 
> >>>  - Hans
> >>> _______________________________________________
> >>> LLVM Developers mailing list
> >>> llvm-dev at lists.llvm.org <mailto:llvm-dev at
lists.llvm.org>         http://llvm.cs.uiuc.edu
<http://llvm.cs.uiuc.edu/>
> >>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
> >>> 
> >> _______________________________________________
> >> LLVM Developers mailing list
> >> llvm-dev at lists.llvm.org <mailto:llvm-dev at
lists.llvm.org>         http://llvm.cs.uiuc.edu
<http://llvm.cs.uiuc.edu/>
> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
> > 
> > 
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org         http://llvm.cs.uiuc.edu
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

On 7 Aug 2015, at 23:17, Tom Stellard via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> I don't have any strong opinions.  Did this actually break anyone's
use
> case or is it just a hypothetical problem?  It doesn't seem like there
> is much value in fixing this until llvm-config is also fixed.
The current 3.7 code breaks for the codebase that I use for teaching.  It uses
RTTI in the AST construction, so can’t be compiled with -fno-rtti, but does need
to use Module, IRBuilder, and friends, so not being able to link against an LLVM
that’s built without RTTI is annoying (it means that the students can’t use LLVM
packages on any OS).

David