This is an archive of the discontinued LLVM Phabricator instance.

[Debuginfod] [Symbolizer] Break debuginfod out of libLLVM.
ClosedPublic

Authored by mysterymath on Jan 27 2022, 3:52 PM.

Details

Summary

Debuginfod can pull in libcurl as a dependency, which isn't appropriate
for libLLVM. (See
https://gitlab.freedesktop.org/mesa/mesa/-/issues/5732).

This change breaks out debuginfod into a separate non-component library
that can be used directly in llvm-symbolizer. The tool can inject
debuginfod into the Symbolizer library via an abstract DebugInfoFetcher
interface, breaking the dependency of Symbolizer on debuinfod.

See https://github.com/llvm/llvm-project/issues/52731

Diff Detail

Event Timeline

mysterymath created this revision.Jan 27 2022, 3:52 PM
mysterymath requested review of this revision.Jan 27 2022, 3:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 27 2022, 3:52 PM
mysterymath edited the summary of this revision. (Show Details)Jan 27 2022, 3:53 PM

Run git clang-format.

phosek added inline comments.Jan 31 2022, 11:52 PM
llvm/include/llvm/DebugInfo/Symbolize/DebugInfoFetcher.h
30

I'd probably go with a simple DIFetcher which better matches the derived types.

33–37

Do we really need the concept? Wouldn't a simple inheritance with virtual functions be sufficient? IIUC the reason for using concept is if you want to opt types that you do not own into the abstract interface, but that's not what we need here since all the types are part of LLVM.

ormris removed a subscriber: ormris.Feb 1 2022, 9:48 AM
mysterymath marked an inline comment as done.

DebugInfoFetcher -> DIFetcher

llvm/include/llvm/DebugInfo/Symbolize/DebugInfoFetcher.h
33–37

The other advantage is removing the requirement for any DIFetcher implementation to depend on Symbolizer. A concept allows the DIFetcher abstraction to properly belong to Symbolizer, i.e., point of use rather than point of implementation.

But, it's a fair amount of syntactic/semantic overhead for just that; my golang proclivities are probably showing a bit. I'm quite ambivalent about this; if this seems like overkill for the gain, let me know.

Simplify concept to interface.

I took a look at the existing concepts in LLVM, and given the overhead in code, they have to be born of deep frustrations. Given that those aren't present here, removed; it should be obvious when they are necessary.

I took a look at the existing concepts in LLVM, and given the overhead in code, they have to be born of deep frustrations. Given that those aren't present here, removed; it should be obvious when they are necessary.

Can't say I'm following this comment - could you describe it in more detail. What existing concepts? What gives the impression they were "Born of deep frustration"?

I took a look at the existing concepts in LLVM, and given the overhead in code, they have to be born of deep frustrations. Given that those aren't present here, removed; it should be obvious when they are necessary.

Can't say I'm following this comment - could you describe it in more detail. What existing concepts? What gives the impression they were "Born of deep frustration"?

Ah, this is in reference to the "concept/model" pattern from Sean Parent's "Inheritance is the base class of evil" talk: https://www.youtube.com/watch?v=bIhUE5uUFOA
It's a way of using templates to get golang-style structural typing interfaces.

It's used in a few places in LLVM; an example is AliasAnalysis.h and IIRC, TargetTransformInfo. You have to duplicate each method two or three times to implement the pattern, so it incurs a large constant overhead in code/maintenance, but the benefits are less clear. Since I don't fully understand why things were done that way for those specific places in LLVM where it's used, I can't effectively argue for the pattern here, so I was just cargo-culting by using it.

I took a look at the existing concepts in LLVM, and given the overhead in code, they have to be born of deep frustrations. Given that those aren't present here, removed; it should be obvious when they are necessary.

Can't say I'm following this comment - could you describe it in more detail. What existing concepts? What gives the impression they were "Born of deep frustration"?

Ah, this is in reference to the "concept/model" pattern from Sean Parent's "Inheritance is the base class of evil" talk: https://www.youtube.com/watch?v=bIhUE5uUFOA
It's a way of using templates to get golang-style structural typing interfaces.

It's used in a few places in LLVM; an example is AliasAnalysis.h and IIRC, TargetTransformInfo. You have to duplicate each method two or three times to implement the pattern, so it incurs a large constant overhead in code/maintenance, but the benefits are less clear. Since I don't fully understand why things were done that way for those specific places in LLVM where it's used, I can't effectively argue for the pattern here, so I was just cargo-culting by using it.

Ah, OK - thanks for the details.

phosek accepted this revision.Feb 8 2022, 1:29 AM

LGTM

This revision is now accepted and ready to land.Feb 8 2022, 1:29 AM
This revision was landed with ongoing or failed builds.Feb 8 2022, 11:14 AM
This revision was automatically updated to reflect the committed changes.