[llvm-dev] Disabling LLVM_ATTRIBUTE_ALWAYS_INLINE for development?

Sun Dec 16 17:01:19 PST 2018

FWIW, I already have patches ready. I'll clean 'em up and post if you want
to run the RFC and maybe propose documentation updates?

On Sun, Dec 16, 2018 at 4:58 PM Davide Italiano <davide at freebsd.org> wrote:

> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20181216/341db907/attachment.html>