llvm dev - Dec 2018 - Disabling LLVM_ATTRIBUTE_ALWAYS_INLINE for development?

On Sat, Dec 15, 2018 at 12:02 PM Vedant Kumar via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
>
> Hi,
>
> > On Dec 15, 2018, at 10:32 AM, Brian Gesiak via llvm-dev <llvm-dev
at lists.llvm.org> wrote:
> >
> > Hello all!
> >
> > I find that using lldb to debug LLVM libraries can be super
> > frustrating, because a lot of LLVM classes, like the constructor for
> > StringRef, are marked LLVM_ATTRIBUTE_ALWAYS_INLINE. So when I attempt
> > to have lldb evaluate an expression that implicitly instantiates a
> > StringRef, I get 'error: Couldn't lookup symbols:
> > __ZN4llvm9StringRefC1EPKc'.
> >
> > As an example, most recently this happened to me when playing around
> > with llvm::AttributeSet, having attached lldb to an opt built with
> > -DCMAKE_BUILD_TYPE="Debug":
> >
> >   (lldb) e $AS.hasAttribute("myattr")
> >   error: Couldn't lookup symbols:
> >     __ZN4llvm9StringRefC1EPKc
> >
> > Despite having built in a "Debug" configuration,
> > LLVM_ATTRIBUTE_ALWAYS_INLINE makes it very difficult to debug LLVM.
> >
> > How do you all deal with or work around this problem?
>
> I don't think there are any great workarounds. The data formatters in
utils/lldbDataFormatters.py can help a bit.
>
>
> > Is there a good
> > way to do so? If not, would anyone object if I sent up a patch to
> > introduce a CMake variable or something to conditionally disable
> > LLVM_ATTRIBUTE_ALWAYS_INLINE? I'd like to be able to turn it off,
but
> > I'm not sure if there's some good reason it needs to stay on
even for
> > debug builds?
>
> IIRC the motivation for using always_inline in debug builds was to make
binaries acceptably fast. IMHO this isn't the right tradeoff because it also
makes binaries frustratingly hard to debug.
>
> Some might object that with clang modules, it should be no trouble for a
debugger to evaluate these kinds of expressions. I've CC'd Raphael who
can speak to this much better than I can. My understanding is that modules may
help, but lldb (and presumably gdb as well?) are not going to learn how to parse
templated code out of C++ modules, instantiate them, and JIT-compile them for
you really soon. That seems like an ongoing, longer-term project.
>
> I'd like to see us move away from using always_inline in core ADT
headers to make debugging easier in the near future.
>
+1 to everything you said. I happened to remember this wasn't always
the case for StringRef, so I did some archeology and found:
[llvm] r247253 - [ADT] Apply a large hammer to StringRef functions:
attribute always_inline.

I'm afraid I missed the commit at the time but there are few remarks I
would like to make:
1) The motivation for this change, at least from what I understand, is
trying to optimize `check-llvm`. As somebody who spends a fair amount
of time tapping fingers while waiting for the tests to run,  I do
believe this is a laudable goal, but I'm uncertain of the results. In
fact, it seems it only saves 1 second (according to the commit
message).
2) I can't speak for anybody else, but I'm not entirely sure a
48-cores box is a common setup (I might be completely wrong on this
one, but I have [and had] access to much more modest hardware). In
other words, I'm not entirely sure the setup that drove the change is
representative of the "typical" llvm developer (for some definition of
"typical").
3) I think the tradeoff here is just not great. I personally devote
much more time debugging llvm (and related subprojects) than running
the testsuite.

YMMV.

Chandler, is there anything that can be done here to make the
experience less frustrating (i.e. revising the decision or reverting)?
The original commit pointed out this was a workaround, but the status
quo is that it makes debugging llvm itself much harder.
 Of course, happy to help.

--
Davide

On Sat, Dec 15, 2018 at 6:13 PM Davide Italiano <davide at freebsd.org>
wrote:
> On Sat, Dec 15, 2018 at 12:02 PM Vedant Kumar via llvm-dev
> <llvm-dev at lists.llvm.org> wrote:
> >
> > Hi,
> >
> > > On Dec 15, 2018, at 10:32 AM, Brian Gesiak via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
> > >
> > > Hello all!
> > >
> > > I find that using lldb to debug LLVM libraries can be super
> > > frustrating, because a lot of LLVM classes, like the constructor
for
> > > StringRef, are marked LLVM_ATTRIBUTE_ALWAYS_INLINE. So when I
attempt
> > > to have lldb evaluate an expression that implicitly instantiates
a
> > > StringRef, I get 'error: Couldn't lookup symbols:
> > > __ZN4llvm9StringRefC1EPKc'.
> > >
> > > As an example, most recently this happened to me when playing
around
> > > with llvm::AttributeSet, having attached lldb to an opt built
with
> > > -DCMAKE_BUILD_TYPE="Debug":
> > >
> > >   (lldb) e $AS.hasAttribute("myattr")
> > >   error: Couldn't lookup symbols:
> > >     __ZN4llvm9StringRefC1EPKc
> > >
> > > Despite having built in a "Debug" configuration,
> > > LLVM_ATTRIBUTE_ALWAYS_INLINE makes it very difficult to debug
LLVM.
> > >
> > > How do you all deal with or work around this problem?
> >
> > I don't think there are any great workarounds. The data formatters
in
> utils/lldbDataFormatters.py can help a bit.
> >
> >
> > > Is there a good
> > > way to do so? If not, would anyone object if I sent up a patch to
> > > introduce a CMake variable or something to conditionally disable
> > > LLVM_ATTRIBUTE_ALWAYS_INLINE? I'd like to be able to turn it
off, but
> > > I'm not sure if there's some good reason it needs to stay
on even for
> > > debug builds?
> >
> > IIRC the motivation for using always_inline in debug builds was to
make
> binaries acceptably fast. IMHO this isn't the right tradeoff because it
> also makes binaries frustratingly hard to debug.
> >
> > Some might object that with clang modules, it should be no trouble for
a
> debugger to evaluate these kinds of expressions. I've CC'd Raphael
who can
> speak to this much better than I can. My understanding is that modules may
> help, but lldb (and presumably gdb as well?) are not going to learn how to
> parse templated code out of C++ modules, instantiate them, and JIT-compile
> them for you really soon. That seems like an ongoing, longer-term project.
> >
> > I'd like to see us move away from using always_inline in core ADT
> headers to make debugging easier in the near future.
> >
>
> +1 to everything you said. I happened to remember this wasn't always
> the case for StringRef, so I did some archeology and found:
> [llvm] r247253 - [ADT] Apply a large hammer to StringRef functions:
> attribute always_inline.
>
> I'm afraid I missed the commit at the time but there are few remarks I
> would like to make:
> 1) The motivation for this change, at least from what I understand, is
> trying to optimize `check-llvm`. As somebody who spends a fair amount
> of time tapping fingers while waiting for the tests to run,  I do
> believe this is a laudable goal, but I'm uncertain of the results. In
> fact, it seems it only saves 1 second (according to the commit
> message).
>
There were a string of related commits here, not just one. They added up at
the time.

> 2) I can't speak for anybody else, but I'm not entirely sure a
> 48-cores box is a common setup (I might be completely wrong on this
> one, but I have [and had] access to much more modest hardware). In
> other words, I'm not entirely sure the setup that drove the change is
> representative of the "typical" llvm developer (for some
definition of
"typical").
>
Not sure this is terribly relevant.... if anything, it minimizes the total
impact. Anyways....

3) I think the tradeoff here is just not great. I personally
devote
> much more time debugging llvm (and related subprojects) than running
> the testsuite.
>
s/testsuite/check-llvm

But this I disagree with, FWIW. Anyways, not sure it is relevant anymore...

Chandler, is there anything that can be done here to make
the
> experience less frustrating (i.e. revising the decision or reverting)?
> The original commit pointed out this was a workaround, but the status
> quo is that it makes debugging llvm itself much harder.
>  Of course, happy to help.
>
I think the time where this made sense has passed us by, and I'm happy to
systematically rip all these things out.

These days, I do not think there is any reasonable performance of
`check-llvm` (or any other `check-foo` w/o optimizations enabled, and debug
info disabled. The former is necessary to get plausible test times, and the
latter is (sadly) necessary to get plausible build times. As a consequence,
I exclusively use optimized (but w/ asserts) builds for running
`check-foo`, and do a separate build if I ever need to debug something. So
I no longer see any benefit from these always-inline attributes, and only
the down sides. I suspect this is true for most developers these days: they
are all downside. If the developer cares about check-foo, they use an
optimized build.

So, as long as we are prepared to add specific guidance to our
documentation that folks hacking on LLVM (and subprojects) should generally
expect to enable -O2 at least to see reasonable test performance, I'm happy
to rip out all the always inline in the whole thing. Even happy to do the
work. Just need the community to agree about the direction.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20181215/9b844c9b/attachment.html>

On Sat, Dec 15, 2018 at 9:37 PM Chandler Carruth via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> On Sat, Dec 15, 2018 at 6:13 PM Davide Italiano <davide at
freebsd.org>
> wrote:
>
>> On Sat, Dec 15, 2018 at 12:02 PM Vedant Kumar via llvm-dev
>> <llvm-dev at lists.llvm.org> wrote:
>> >
>> > Hi,
>> >
>> > > On Dec 15, 2018, at 10:32 AM, Brian Gesiak via llvm-dev <
>> llvm-dev at lists.llvm.org> wrote:
>> > >
>> > > Hello all!
>> > >
>> > > I find that using lldb to debug LLVM libraries can be super
>> > > frustrating, because a lot of LLVM classes, like the
constructor for
>> > > StringRef, are marked LLVM_ATTRIBUTE_ALWAYS_INLINE. So when I
attempt
>> > > to have lldb evaluate an expression that implicitly
instantiates a
>> > > StringRef, I get 'error: Couldn't lookup symbols:
>> > > __ZN4llvm9StringRefC1EPKc'.
>> > >
>> > > As an example, most recently this happened to me when playing
around
>> > > with llvm::AttributeSet, having attached lldb to an opt built
with
>> > > -DCMAKE_BUILD_TYPE="Debug":
>> > >
>> > >   (lldb) e $AS.hasAttribute("myattr")
>> > >   error: Couldn't lookup symbols:
>> > >     __ZN4llvm9StringRefC1EPKc
>> > >
>> > > Despite having built in a "Debug" configuration,
>> > > LLVM_ATTRIBUTE_ALWAYS_INLINE makes it very difficult to debug
LLVM.
>> > >
>> > > How do you all deal with or work around this problem?
>> >
>> > I don't think there are any great workarounds. The data
formatters in
>> utils/lldbDataFormatters.py can help a bit.
>> >
>> >
>> > > Is there a good
>> > > way to do so? If not, would anyone object if I sent up a
patch to
>> > > introduce a CMake variable or something to conditionally
disable
>> > > LLVM_ATTRIBUTE_ALWAYS_INLINE? I'd like to be able to turn
it off, but
>> > > I'm not sure if there's some good reason it needs to
stay on even for
>> > > debug builds?
>> >
>> > IIRC the motivation for using always_inline in debug builds was to
make
>> binaries acceptably fast. IMHO this isn't the right tradeoff
because it
>> also makes binaries frustratingly hard to debug.
>> >
>> > Some might object that with clang modules, it should be no trouble
for
>> a debugger to evaluate these kinds of expressions. I've CC'd
Raphael who
>> can speak to this much better than I can. My understanding is that
modules
>> may help, but lldb (and presumably gdb as well?) are not going to learn
how
>> to parse templated code out of C++ modules, instantiate them, and
>> JIT-compile them for you really soon. That seems like an ongoing,
>> longer-term project.
>> >
>> > I'd like to see us move away from using always_inline in core
ADT
>> headers to make debugging easier in the near future.
>> >
>>
>> +1 to everything you said. I happened to remember this wasn't
always
>> the case for StringRef, so I did some archeology and found:
>> [llvm] r247253 - [ADT] Apply a large hammer to StringRef functions:
>> attribute always_inline.
>>
>> I'm afraid I missed the commit at the time but there are few
remarks I
>> would like to make:
>> 1) The motivation for this change, at least from what I understand, is
>> trying to optimize `check-llvm`. As somebody who spends a fair amount
>> of time tapping fingers while waiting for the tests to run,  I do
>> believe this is a laudable goal, but I'm uncertain of the results.
In
>> fact, it seems it only saves 1 second (according to the commit
>> message).
>>
>
> There were a string of related commits here, not just one. They added up
> at the time.
>
>
>> 2) I can't speak for anybody else, but I'm not entirely sure a
>> 48-cores box is a common setup (I might be completely wrong on this
>> one, but I have [and had] access to much more modest hardware). In
>> other words, I'm not entirely sure the setup that drove the change
is
>> representative of the "typical" llvm developer (for some
definition of
>
> "typical").
>>
>
> Not sure this is terribly relevant.... if anything, it minimizes the total
> impact. Anyways....
>
> 3) I think the tradeoff here is just not great. I personally devote
>> much more time debugging llvm (and related subprojects) than running
>> the testsuite.
>>
>
> s/testsuite/check-llvm
>
> But this I disagree with, FWIW. Anyways, not sure it is relevant anymore...
>
> Chandler, is there anything that can be done here to make the
>> experience less frustrating (i.e. revising the decision or reverting)?
>> The original commit pointed out this was a workaround, but the status
>> quo is that it makes debugging llvm itself much harder.
>>  Of course, happy to help.
>>
>
> I think the time where this made sense has passed us by, and I'm happy
to
> systematically rip all these things out.
>
> These days, I do not think there is any reasonable performance of
> `check-llvm` (or any other `check-foo` w/o optimizations enabled, and debug
> info disabled.
>
For some value of "reasonable" - FWIW I (& sounds like a few
others at
least) use a straight Debug (unoptimized, (split) debug info enabled) build
& regularly run the binaries in a debugger.

> The former is necessary to get plausible test times, and the latter is
> (sadly) necessary to get plausible build times.
>
Curious what your definition/line is for those? (& I guess I disagree,
since I find a debug build to be sufficient/the right tradeoff for my needs)

All that said, I've been debugging LLVM regularly like this using GDB for
years and didn't notice this regression in functionality probably because
I'm used to GDB's lack of C++ understanding, so I probably learned early
on
that it couldn't handle some cases like this so I rarely try to call code
short of LLVM's 'dump' functions from within the debugger.

I do rather like the idea of having a different always_inline variant
that's for "yeah, this is really necessary for plausible performance,
but
not for correctness" that, combined with -g (OK, some variant of -g (a -f
modifier, maybe) perhaps, so we can maintain the "-g doesn't affect
codegen" invariant) causes even functions that are inlined away to be
emitted (this could extend to functions not marked inline too, of course -
but we'd still need the variant attribute to ensure we didn't emit
standalone definitions of functions that make no sense/can't be correctly
executed when not inlined).

> As a consequence, I exclusively use optimized (but w/ asserts) builds for
> running `check-foo`, and do a separate build if I ever need to debug
> something. So I no longer see any benefit from these always-inline
> attributes, and only the down sides. I suspect this is true for most
> developers these days: they are all downside. If the developer cares about
> check-foo, they use an optimized build.
>
> So, as long as we are prepared to add specific guidance to our
> documentation that folks hacking on LLVM (and subprojects) should generally
> expect to enable -O2 at least to see reasonable test performance, I'm
happy
> to rip out all the always inline in the whole thing. Even happy to do the
> work. Just need the community to agree about the direction.
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> 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/20181216/ae46779e/attachment-0001.html>

On Sat, Dec 15, 2018 at 9:37 PM Chandler Carruth <chandlerc at gmail.com>
wrote:
>
> On Sat, Dec 15, 2018 at 6:13 PM Davide Italiano <davide at
freebsd.org> wrote:
>>
>> On Sat, Dec 15, 2018 at 12:02 PM Vedant Kumar via llvm-dev
>> <llvm-dev at lists.llvm.org> wrote:
>> >
>> > Hi,
>> >
>> > > On Dec 15, 2018, at 10:32 AM, Brian Gesiak via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
>> > >
>> > > Hello all!
>> > >
>> > > I find that using lldb to debug LLVM libraries can be super
>> > > frustrating, because a lot of LLVM classes, like the
constructor for
>> > > StringRef, are marked LLVM_ATTRIBUTE_ALWAYS_INLINE. So when I
attempt
>> > > to have lldb evaluate an expression that implicitly
instantiates a
>> > > StringRef, I get 'error: Couldn't lookup symbols:
>> > > __ZN4llvm9StringRefC1EPKc'.
>> > >
>> > > As an example, most recently this happened to me when playing
around
>> > > with llvm::AttributeSet, having attached lldb to an opt built
with
>> > > -DCMAKE_BUILD_TYPE="Debug":
>> > >
>> > >   (lldb) e $AS.hasAttribute("myattr")
>> > >   error: Couldn't lookup symbols:
>> > >     __ZN4llvm9StringRefC1EPKc
>> > >
>> > > Despite having built in a "Debug" configuration,
>> > > LLVM_ATTRIBUTE_ALWAYS_INLINE makes it very difficult to debug
LLVM.
>> > >
>> > > How do you all deal with or work around this problem?
>> >
>> > I don't think there are any great workarounds. The data
formatters in utils/lldbDataFormatters.py can help a bit.
>> >
>> >
>> > > Is there a good
>> > > way to do so? If not, would anyone object if I sent up a
patch to
>> > > introduce a CMake variable or something to conditionally
disable
>> > > LLVM_ATTRIBUTE_ALWAYS_INLINE? I'd like to be able to turn
it off, but
>> > > I'm not sure if there's some good reason it needs to
stay on even for
>> > > debug builds?
>> >
>> > IIRC the motivation for using always_inline in debug builds was to
make binaries acceptably fast. IMHO this isn't the right tradeoff because it
also makes binaries frustratingly hard to debug.
>> >
>> > Some might object that with clang modules, it should be no trouble
for a debugger to evaluate these kinds of expressions. I've CC'd Raphael
who can speak to this much better than I can. My understanding is that modules
may help, but lldb (and presumably gdb as well?) are not going to learn how to
parse templated code out of C++ modules, instantiate them, and JIT-compile them
for you really soon. That seems like an ongoing, longer-term project.
>> >
>> > I'd like to see us move away from using always_inline in core
ADT headers to make debugging easier in the near future.
>> >
>>
>> +1 to everything you said. I happened to remember this wasn't
always
>> the case for StringRef, so I did some archeology and found:
>> [llvm] r247253 - [ADT] Apply a large hammer to StringRef functions:
>> attribute always_inline.
>>
>> I'm afraid I missed the commit at the time but there are few
remarks I
>> would like to make:
>> 1) The motivation for this change, at least from what I understand, is
>> trying to optimize `check-llvm`. As somebody who spends a fair amount
>> of time tapping fingers while waiting for the tests to run,  I do
>> believe this is a laudable goal, but I'm uncertain of the results.
In
>> fact, it seems it only saves 1 second (according to the commit
>> message).
>
>
> There were a string of related commits here, not just one. They added up at
the time.
>
>>
>> 2) I can't speak for anybody else, but I'm not entirely sure a
>> 48-cores box is a common setup (I might be completely wrong on this
>> one, but I have [and had] access to much more modest hardware). In
>> other words, I'm not entirely sure the setup that drove the change
is
>> representative of the "typical" llvm developer (for some
definition of
>>
>> "typical").
>
>
> Not sure this is terribly relevant.... if anything, it minimizes the total
impact. Anyways....
>
>> 3) I think the tradeoff here is just not great. I personally devote
>> much more time debugging llvm (and related subprojects) than running
>> the testsuite.
>
>
> s/testsuite/check-llvm
>
> But this I disagree with, FWIW. Anyways, not sure it is relevant anymore...
>
>> Chandler, is there anything that can be done here to make the
>> experience less frustrating (i.e. revising the decision or reverting)?
>> The original commit pointed out this was a workaround, but the status
>> quo is that it makes debugging llvm itself much harder.
>>  Of course, happy to help.
>
>
> I think the time where this made sense has passed us by, and I'm happy
to systematically rip all these things out.
>
> These days, I do not think there is any reasonable performance of
`check-llvm` (or any other `check-foo` w/o optimizations enabled, and debug info
disabled. The former is necessary to get plausible test times, and the latter is
(sadly) necessary to get plausible build times. As a consequence, I exclusively
use optimized (but w/ asserts) builds for running `check-foo`, and do a separate
build if I ever need to debug something. So I no longer see any benefit from
these always-inline attributes, and only the down sides. I suspect this is true
for most developers these days: they are all downside. If the developer cares
about check-foo, they use an optimized build.
>
> So, as long as we are prepared to add specific guidance to our
documentation that folks hacking on LLVM (and subprojects) should generally
expect to enable -O2 at least to see reasonable test performance, I'm happy
to rip out all the always inline in the whole thing. Even happy to do the work.
Just need the community to agree about the direction.
Sounds like we have a plan then. I'm happy to volunteer. I guess we
just need to wait some time to see if people have objections, and then
send an RFC to the mailing list once the patches are ready.

Thanks for your time.

-- 
Davide

"There are no solved problems; there are only problems that are more
or less solved" -- Henri Poincare