In D71932#1827849, @ikudrin wrote:

Thanks for the review!

In D71932#1827601, @dblaikie wrote:

Looks good - future work done as separate patches for separable changes/diagnostics would probably be good (& if you've got some broader goals in terms of what functionality you need/would like in llvm-dwarfdump, an email to llvm-dev explaining the broader goals (do they overlap with James Henderson's goals, improving the line table error handling?) would probably be good - to link to in future reviews, etc)

My main intention is to add support for 64-bit DWARF.

In D72155#1813778, @jhenderson wrote:

In D72155#1813355, @dblaikie wrote:

Out of curiosity: what's your broader goal with this work? (it'll help understand what's in-scope and out of scope, and better understand the framing when reviewing changes)

There's two parts to this:

1. Making it easier for consumers to continue and try to do something with slightly bad output. One of the problems with using the unrecoverable errors is that it prevents people even trying to iterate over later tables.
2. (this one's not specific to this patch, but some other patches in this area I've been doing are related): we have some local code that uses the DebugInfo library to read a line table. In order for this code to be sound, we need to make sure the line table makes sense. The parser in its current state doesn't pick up on a number of bad situations, so we added some local patches to detect these other errors (local because we didn't have time at that point to try to push them into the open source). We'd like to now get these into the opensource library, to avoid merge conflicts.

Let's try that again - guess I'd already approved this before.

Sounds good - thanks!

Looks good - future work done as separate patches for separable changes/diagnostics would probably be good (& if you've got some broader goals in terms of what functionality you need/would like in llvm-dwarfdump, an email to llvm-dev explaining the broader goals (do they overlap with James Henderson's goals, improving the line table error handling?) would probably be good - to link to in future reviews, etc)

Looks good - James, I might've asked this on another thread - but could you write up a brief goals/design email of what your overall goal is with these patches? There seem to be enough of them that it'd be helpful to link to in each review (and summarize where the patch fits in the broader scheme of things) & provide framing when reviewing patches to better understand the overall goals.

Ah, bother - good catch!

Looks good - thanks!

In D72590#1825399, @dblaikie wrote:

I've committed a generalization of this (applying the same logic to all instances of NumLowBitsAvailable) in 65eb74e94b414fcde6bfa810d1c30c7fcb136b77 - so this review can be closed or abandoned.

Looks good - thanks!

I've committed a generalization of this (applying the same logic to all instances of NumLowBitsAvailable) in 65eb74e94b414fcde6bfa810d1c30c7fcb136b77 - so this review can be closed or abandoned.

Please implement the dwarfdump support first (this patch can be reviewed during that time - but shouldn't be committed without tests & ideally those tests should be written using dwarfdump (sometimes there are exceptions for especially complicated dumping situations, where raw assembly testing is used instead, but I don't think it's necessary to make that tradeoff here))

Sounds good -t hanks for all the context. Previously this might've been an effort to keep the dependencies of a core header like Module.h small, but since it now has the dependencies this relies on - this change isn't making the situation worse in any way I can think of.

Looks good -t hanks!

The usual "naming of things" - are there flags with similar functionality in other tools (like llvm-symbolizer) that might provide inspiration for consistent naming here (perhaps the name you've chosen already was inspired by that)?

In D71451#1822497, @Jac1494 wrote:

I'd be curious to the answer to David's questions. If the size increase is because of unused extern variables coming in from libc or something then it doesn't seem worth the cost.

For above case clang size is increase because ,it is difference between clang build without "-fstandalone-debug" option and clang build with "-fstandalone-debug" option and both build contain change D71451 and D71599 . So for clang build with "-fstandalone-debug" option size will be more because it will add debuginfo.

And to check impact of my change on clang i have build clang with and without D71451 and D71599 change(testcases are not included).

Size of clang without D71451 and D71599 change and with option "-fstandalone-debug":-
=======================================================================
…
.comment 159 0
.debug_str 3994952 0
.debug_loc 941 0
.debug_abbrev 12754 0
.debug_info 2223641 0
.debug_ranges 46592 0
.debug_line 153901 0
.note.gnu.gold-version 28 0
Total 6827932

Size of clang with D71451 and D71599 change and with option "-fstandalone-debug":-
======================================================================
…
.comment 159 0
.debug_str 3994894 0
.debug_loc 941 0
.debug_abbrev 12746 0
.debug_info 2223617 0
.debug_ranges 46592 0
.debug_line 153865 0
.note.gnu.gold-version 28 0
Total 6827806

Size of clang with D71451 and D71599 is reduced.

While the related design discussions continue - the patch itself is good/correct & there's nothing much to be done about the address pool size/relocations increase for now, for GDB at least, which is what I care about.

(just a general comment that this code review should be only in service of the design discussion happening on llvm-dev - please don't approve/commit this without closing out the design discussion there if there are actionable conclusions from this review)

Looks good - thanks for your patience!

In D71732#1813229, @aprantl wrote:

Some mysterious CFI failures I haven't looked into yet, but were pretty clearly blamed on this change & went away with a revert.

Because the flag was previously not (de)serialized and now shows up in LTO builds where it was dropped before?

Could you provide a small example of the problem here? LLVM does emit debug info for used enums (granted, if it's used as a template parameter it looks like LLVM does not emit them - but if it's used in a runtime evaluated expression it does/should & it looks like NumLowBitsAvailable is used in that way pretty regularly)

Looks good - thanks!

In D72427#1818322, @rnk wrote:

Total object file size on Windows, compiling with RelWithDebInfo:

before: 4,257,448 kb
after: 2,104,963 kb

And on Linux

before: 9,225,140 kb
after: 4,387,464 kb

These numbers are amazing!

I made a summary of Amy's list of types that become incomplete here: https://reviews.llvm.org/P8184
It collapses template parameters and counts up instantiations.

Looks pretty good to me - if you could commit the debug info level refactoring separately/up-front, and maybe the test case could be simplified a bit. Looking forward to seeing what comes of this option, analysis of missing types, etc.

Probably needs test coverage (perhaps check the commits that added other feature tests - there's probably somewhere you can plugin tests in a similar way to plugging in the feature tests themselves)

I ran some size analysis (& hadn't realized that this special case was only done in the DWARFv5 codepath... that leads me to wonder about size impact in DWARFv4, so maybe I'll need to go back and run some general size analysis cost of these call sites).

Looks good - thanks!

In D72337#1814108, @jhenderson wrote:

In D72337#1813202, @dblaikie wrote:

In D72337#1811669, @jhenderson wrote:

Added unit test case that would trigger the assertion mentioned in the description. I'm not entirely convinced that this test case is worthwhile, since with the change in how size() is calculated, it really is moot (e.g. I wouldn't have needed it if the size() function had always been the way I'm proposing).

Ah, I was more expecting an end-to-end test (with llvm-dwarfdump or similar) when you said in the patch description "The old behaviour could cause an assertion in the debug line parser"

What were the circumstances where this assertion would fail? Was the failure reachable with llvm-dwarfdump, or only via a unit test/non-production API usage?

Oh, sorry for misunderstanding. We have a local test for a local patch in LLD that makes some use of the debug information, but this patch doesn't create the data extractor with a section, which meant that it ran into this assertion. I checked, and I can't find any other usage of this function with a data extractor constructed without a section.

Thanks for looking into this!

Seems good

As mentioned in another review - I'm not sure "assume bigger is correct" is a great strategy & not sure there's a lot of value in continuing in the face of a conflict like that. What sort of situations really benefit from such parsing optimism?

I guess these error messages could be more precise about what's invalid & why it's invalid. In this case I guess it's invalid because the DW_LNCT_MD5 has a form that contains only one byte, not large enough to encode a whole MD5 sum?

In D72427#1812954, @akhuang wrote:

What's the plan for this? Is it still in an experimental stage, with the intent to investigate the types that are no longer emitted unedr the flag & explain why they're missing (& either have a justification for why that's acceptable, or work on additional heuristics to address the gaps?)?

If so, I'd probably rather this not be a full driver flag - if it's a reliable way to reduce debug info size (like the existing heuristics under -fstandalone-debug*) it should be rolled into -fno-standalone-debug behavior, and if it's not fully fleshed out yet, I think an -Xclang flag would be more suitable for the experimental phase.

Pretty much, I think the plan is to investigate further and maybe have more people try it. The -Xclang flag seems reasonable. Do you have thoughts on whether the added DebugInfoKind level makes sense?

Out of curiosity: what's your broader goal with this work? (it'll help understand what's in-scope and out of scope, and better understand the framing when reviewing changes)

In D72337#1811669, @jhenderson wrote:

Added unit test case that would trigger the assertion mentioned in the description. I'm not entirely convinced that this test case is worthwhile, since with the change in how size() is calculated, it really is moot (e.g. I wouldn't have needed it if the size() function had always been the way I'm proposing).

Padding stuff seems like overkill to me, hardcoding the actual value seems like it'd probably be fine, but I don't mind so much.

In D71732#1812937, @aprantl wrote:

@dblaikie: I just saw you reverted https://reviews.llvm.org/rGc51b45e32ef7f35c11891f60871aa9c2c04cd991 ? What happened?

What's the plan for this? Is it still in an experimental stage, with the intent to investigate the types that are no longer emitted unedr the flag & explain why they're missing (& either have a justification for why that's acceptable, or work on additional heuristics to address the gaps?)?

The old behaviour could cause an assertion in the debug line parser

What about going the other way, and making sure instructions in frame setup that need labels get them?

Could we do this the other way around - and "AND LINKER_IS_GNULD"? Because so far as I know gold has reasonable memory usage too (& LD64), etc. I /think/ it's just GNULD that's the outlier here. (I can't speak for SOLARISLD, admittedly)

Sounds good to me - but probably good to get someone who works on the MSVC platform (like @rnk ) to confirm.

Seems OK to me, but @aprantl - could you take a look too since I don't often use/interact with the debuginfo-tests. Do you think this is something worth having/appropriate to have there?

Looks pretty good to me - couple of minor things. If you want to check with @rnk (best point of contact on the inalloca feature in general), that'd probably be good.

In D72197#1806523, @MaskRay wrote:

In D72197#1806495, @dblaikie wrote:

I think the question I have: What is the value (I don't mean to assume there is no value, but I want to understand the specific use case) of supporting this if the full generality of symbol interposition is not supported? (is there a well defined use case that is usable with this patch and doesn't need the full generality? Or is the use case going to be subtly broken if it changes even slighlty and happens to trip over another part of LLVM that doesn't support symbol interposition? (in which case this is a great patch, but only as part of an effort to fully support the general case))

@dblaikie I came from a ppc64 use case https://reviews.llvm.org/D71639#1803561 where I could not construct a good test due to the MC issue this patch intends to fix. The advantages include at least the following:

• Improve compatibility with GNU as. This improves portability of hand-written assembly.

In D72197#1806478, @MaskRay wrote:

In D72197#1805964, @sfertile wrote:

In D72197#1805438, @bd1976llvm wrote:

I wonder if is this really a bug or intentional. Clang and LLVM seem to ignore the possibility of symbol interposition (see: -fno-semantic-interposition). What is the LLVM policy for this?

I had a talk about this once I believe with @hfinkel (or maybe @dblaikie), and this behavior was intentional. Essentially LLVM may perform IPO between the 2 functions, in which case its no longer appropriate to allow the callee to be preempted at runtime. The back-ends aren't consistent on this behaviour either. For example:

__attribute__((noinline))
int bar(int i) {
    return i;
}


int caller(int i) {
    if (bar(i) == i)
      return 0;

    return 1;
}

Compiling this for PowerPC and AArch64 Linux and -fPIC I get no relocation for the call. Compiling for X86_64 Linux and -fPIC I do get a relocation for the call. So it seems to be pick your poison: Emit a relocation and expose a potential runtime problem, or don't emit a relocation and break ELF interposition semantics. I'm also not sure if the optimizer actually skips all IPO when the functions are in different sections, so we might end up with both broken shared object interposition and potentially unsafe codegen 😦

Behavior without this patch.

• clang -target {aarch64,ppc64le} -fPIC -c => no relocation referencing bar
• clang -target {aarch64,ppc64le} -fPIC -c -ffunction-sections => a branch relocation referencing bar
  
  Look, we are already inconsistent. With this patch, we will consistently emit a relocation. I thinks this patch makes the situation better.

Bit verbose, but sounds alright.

Ah, thanks for that - seems I wrote this as I had /wanted/ ArrayRef to be implemented, rather than how it ended up being implemented. (when MutableArrayRef was proposed, I had proposed changing ArrayRef to be mutable and introducing ConstArrayRef but folks didn't want the churn - guess that stuck in my head a bit).

In D70600#1803899, @hintonda wrote:

In D70600#1803789, @dblaikie wrote:

In D70600#1802516, @sammccall wrote:

In D70600#1802227, @lhames wrote:

In D70600#1801726, @sammccall wrote:

FWIW working on a codebase that heavily uses both Error and threads, the side-table seems fairly a bit terrifying. Even if the thread safety issues are fixed, do we really want to pay for mutex lock/unlock to create errors, to save a pointer?

thread_local might be an option, though as I understand llvm/Support may have to be portable to environments where that doesn't work well.

An atomic pointer would work too. This cost is only paid on failure values and these are already heap allocated, so I'm I wouldn't expect the overhead to be prohibitive either way.

I don't follow this - are you saying if heap allocation is OK, locking a mutex must be too, as it's cheaper?
Locking/unlocking a shared global mutex can certainly be more expensive than malloc if there's a lot of contention. malloc implementations typically go to some lengths to avoid a global lock for this reason.

Example workload: clangd typically runs ~30 threads for background indexing, each thread frequently calls functions returning Expected<T> and recovers from errors (Error is documented as being for recoverable errors). It'd be a big surprise if this sort of use of Error led to contention/serialization on these worker threads.

Note this feature's only intended to be opt-in for debugging purposes, so for production use cases it shouldn't be a huge deal (though could be cheaper/free if it was a build-time setting rather than something you could opt into/out of at runtime - perhaps that's the point you're making? Though we do check various globals (cl::opts, etc) in a fair number of codepaths, I think, such that adding one doesn't seem likely to be super problematic?). (as much as I agree with you that I'd rather not have a side table)

Part of the reason for this patch was a need to provide file/line number for errors that lead to aborts via llvm::cantFail. This can be very important when debugging failing bots for which you have not access and are unable to reproduce. Since this solution is too expensive to turn on all the time, it's doubtful it'll be enabled on any bots, which makes it useless for my primary use case.

That doesn't mean it wouldn't be useful in cases where you can reproduce locally, but that wasn't my initial motivation. I'm also wary of the side-table.

In D70600#1802516, @sammccall wrote:

In D70600#1802227, @lhames wrote:

In D70600#1801726, @sammccall wrote:

FWIW working on a codebase that heavily uses both Error and threads, the side-table seems fairly a bit terrifying. Even if the thread safety issues are fixed, do we really want to pay for mutex lock/unlock to create errors, to save a pointer?

thread_local might be an option, though as I understand llvm/Support may have to be portable to environments where that doesn't work well.

An atomic pointer would work too. This cost is only paid on failure values and these are already heap allocated, so I'm I wouldn't expect the overhead to be prohibitive either way.

I don't follow this - are you saying if heap allocation is OK, locking a mutex must be too, as it's cheaper?
Locking/unlocking a shared global mutex can certainly be more expensive than malloc if there's a lot of contention. malloc implementations typically go to some lengths to avoid a global lock for this reason.

Example workload: clangd typically runs ~30 threads for background indexing, each thread frequently calls functions returning Expected<T> and recovers from errors (Error is documented as being for recoverable errors). It'd be a big surprise if this sort of use of Error led to contention/serialization on these worker threads.

In D70628#1803671, @lhames wrote:

In D70628#1762343, @hintonda wrote:

In D70628#1762116, @hintonda wrote:

In D70628#1761330, @lhames wrote:

...
I think the best long term solution to this problem is to teach llvm-symbolize how to cope with debug info in objects, rather than requiring a dSYM. An intermediate step (possibly generically useful) would be to add an option to llvm-symbolize to run llvm-dsymutil if there's no dSYM available.

I suppose that might be useful for individuals, but I don't think it would be a good idea for the bots, which is my primary target.

My last comment concerned the intermediate step. I think your long-term solution would be great.

If the option is added (and LLVM's stack symbolication call-out knows to use it) then I think it should work on the bots too, right? The only issue would be that crashing test cases would be slightly slower (since we'd have to produce the dSYMs).

In D70600#1802235, @lhames wrote:

Oh, I remember part of my motivation for designing it this way: While failure values are expected to be expensive, we don't want to make them unnecessarily expensive, which is why the error-geneation-point trace is under a flag: You don't want to stack-trace every error that is ever generated. However, making this conditional means we need to check a global flag to decide whether we *should* do the stack trace. Once you're doing that, you may as well make your "flag" a pointer to the side table.

Might want to use CHECK-NEXT as you were before and/or maybe some CHECK-NOT: DW_TAG between the TAG/AT/AT triples, to make sure you aren't checking attributes in some unintended TAG, etc.

Hey - thanks for this! Does look like it reproduces in modules:

In D71839#1801654, @JDevlieghere wrote:

In D71839#1799149, @dblaikie wrote:

In D71839#1795483, @avl wrote:

Not sure if this is better/worse off as a subdirectory of lib/CodeGen, rather than as a top level library on its own (lib/DWARFOptimizer). Figured I'd mention it, but mostly leave it up to the other dsymutil folks to make the choices here.

My idea was that DWARFOptimizer does generation task. F.e. AsmPrinter - generates asm and it is under CodeGen. DWARFOptimizer generates optimized DWARF. That is why I put it under CodeGen.
Though if it is better to put it on top level - lib/DWARFOptimizer - I am OK to do this.

@aprantl @JDevlieghere @probinson any preferences here? If I've got to make the call I'll probably pick lib/DWARFOptimizer or lib/DWARFLinker (probably the latter)

My personal preference goes to lib/DWARFLinker.

I take it there's no way to write a portable test for this patch? (one that doesn't have to run on the specific hardware, but still demonstrates the presence of this newly added support?)

In D71839#1795483, @avl wrote:

Not sure if this is better/worse off as a subdirectory of lib/CodeGen, rather than as a top level library on its own (lib/DWARFOptimizer). Figured I'd mention it, but mostly leave it up to the other dsymutil folks to make the choices here.

My idea was that DWARFOptimizer does generation task. F.e. AsmPrinter - generates asm and it is under CodeGen. DWARFOptimizer generates optimized DWARF. That is why I put it under CodeGen.
Though if it is better to put it on top level - lib/DWARFOptimizer - I am OK to do this.

Was this fixing an existing buildbot/test failure? (if not, it should probably have a test? If it did fix an existing failure - could you mention what that failure was?)

In D71026#1785025, @arichardson wrote:

If I build on macOS with cmake -GNinja -DBUILD_SHARED_LIBS=ON -DCMAKE_BUILD_TYPE=Debug -DLLVM_ENABLE_PROJECTS=llvm;clang;lld;compiler-rt;libcxx;libcxxabi -DBUILD_SHARED_LIBS=ON -DLLVM_ENABLE_MODULES=ON, I get lots of linker errors building e.g. libclangASTDiff.dylib due to using the internal ASTMatchers header inside the Tooling/Transformer headers.

This is probably due to https://bugs.llvm.org/show_bug.cgi?id=23521 mentioned in the previous commits that added the exclude directives.

Could these tests all be in one run? (ie: even though the length is "invalid" in terms of parsing the contents of the section - but valid enough to jump to the next contribution and parse that - except, I guess, at an invalid 64 bit length perhaps, because that might be too long to be bothered testing - so perhaps that could be the last test case in the file)

yeah, as @probinson said, at some point someone could add a callback error filter here to decide what's a fatal error and what isn't - but this seems reasonable for now

Looks good - thanks!

In D70696#1795427, @yonghong-song wrote:

@dblaikie The root cause has been identified by @rnk . A clang plugin is used which requires the following patch:

https://github.com/llvm/llvm-project/commit/5128026467cbc17bfc796d94bc8e40e52a9b0752

which has been merged. The original extern variable debug info type has been relanded. So we are good. Thanks!