In D87272#2264494, @MaskRay wrote:

In D87272#2264430, @dblaikie wrote:

In D87272#2264421, @MaskRay wrote:

In D87272#2264383, @dblaikie wrote:

In D87272#2263758, @MaskRay wrote:

In D87272#2261876, @dblaikie wrote:

In D87272#2261812, @MaskRay wrote:

In D87272#2261768, @dblaikie wrote:

Is performance important in this case? (admittedly, some folks might have builds full of warnings they aren't paying attention to & so they still care about performance then?) Generally the idea with clang/etc is that anything that produces user output (other than the successfully compiled/linked file) doesn't need to be fast (in compile/link-time terms) because it'll take /way/ longer for a user to read and think about the error message than the slight increase in execution time.

The nicer thing is that when running under strace/truss, there will be just one write for one diagnostic.

Is that true of many other tools? (I wouldn't've expected it'd be true of clang, for instance - or probably other LLVM utilities) - is it especially helpful?

The motivation was to reduce unneeded ::write calls.

Sounds like unnecessary/premature optimization, though?

I don't think it is premature optimization. When a --symbol-ordering-file has absent entries or when --call-graph-ordering-file has incorrect entries, the terminal output can be quite slow. This change can make it much faster by also making things just better (one write syscall for one line of diagnostic).

How much faster is the performance if you user disables the warning they have chosen not to fix? (if they're fixing it, the time taken to fix it will be a lot longer than the link time penalty)

https://reviews.llvm.org/D87121#2260177 I don't use it as a motivation for the patch. I created this patch because I think it is the right thing to do.

In D87732#2278612, @jhenderson wrote:

In D87732#2278196, @phosek wrote:

In D87732#2275943, @jhenderson wrote:

I'm not convinced this is really correct. After all, a path with mixed separators (e.g. '/my/path\to\foo') could be a Windows path rooted at the current drive. This mixed style can sometimes happen when things get concatenated together, so I don't think it's compeletely unreasonable.

I'm also concerned that if we guess wrong, the behaviour will end up breaking. For example, if the path were a Windows path "/my/path\../to/foo", I'd expect a remove_dots call (without worrying about slash normalisation) to result in "/my/to/foo", but if the code is using Posix style, will it actually result in the path being left unchanged?

I can think of two competing alternatives:

1. add an option to remove_dots to not do the separator canonicalization. I'm not sure whether this can be done unambiguously however - which separator should be removed when a directory is removed due to dots? The one before the removed parts? After them?
2. change all '\' to '/' unconditionally. However, this might break Linux paths with '\' in.

None of these solutions is perfect and I'm not even sure if there's one. Even if we improve remove_dots, there's still the existing issue with sys::path::append: which separator should we use when combining compilation dir, include dir and the file name.

Maybe we should instead update Clang to do the normalization when generating debug info and always use /, and then always use / unconditionally when combining paths on the consumer side (but don't do any normalization)?

You're probably right about no solution being perfect. I'm not a fan of using '/' on Windows, mostly because it looks weird, but also because at least in the past for Windows cmd, '/' paths didn't auto-complete properly. It's only a minor gripe though (and not really an issue nowadays since I mostly use PowerShell). Normalisation to at least remove dots would make a lot of sense (strings become shorter), and possibly even normalising the separator direction does too, since that would make it more likely paths end up being identical and therefore poolable in .debug_str/.debug_line_str.

sys::path::append could probably unconditionally be Posix, since such paths will also be valid Windows paths always. Maybe we need some way of communicating target platform as an alternative to native style? If that were a command-line option, the caller at least would know which style it needs to work with.

I'd like to hear others opinions - @dblaikie?

OK, having a go at reviewing this (though likely you're about in the best position to assess this, Eli)...

Looks good - I don't feel that strongly about the split or merged commands.

Looks good - thanks!

In D81865#2293059, @MaskRay wrote:

@froydnj The committed version rG31a3c5fb45b78bdaa78d94ffcc9258e839002016 appears to be very different from the review. I guess next time your probably can upload the diff again if it is very diffierent

Went with D87935 instead.

In D87935#2289415, @jhenderson wrote:

In D87935#2285841, @dblaikie wrote:

In D87935#2284630, @jhenderson wrote:

If I had to pick between the two existing options, I think I have a marginal preference for this one, since it requires less code and is less surprising to someone who might not be too familiar with the code - I would expect a list of units to be in offset order naturally, and not expect to have to go through some sort of sorting procedure in the future.

Fair - appreciate the perspective. One other wrinkle here is that if we support the "type_units()" API (I don't think anyone's actually using it, so I'd probably delete it for now) it'll need to use a fancy iterator solution too - concat_iterator + filter_iterator to take the v5 type units out of the debug_info units, and concat that with the v4 type units from debug_types.

I'm happy for that little extra complexity. I think it intuitively makes sense, even if it does mean messing about a little in the code.

hmm, possibly more words in the patch description (& IR documentation). of this and the previous patch - what is implicit_pointer V explicit_pointer? Maybe some examples in the patch description and IR documentation about how they could be used?

Generally looks good to me!

In D68620#2290273, @probinson wrote:

TL;DR: It's all good.

As I worked through the calculations again myself, I realized I had forgotten that both the start and end address in a v4 entry were relocated. (The spec says neither of them are; they are supposed to be relative to the CU base address. But nobody actually does it that way.)

Seems good to me - minor tidy up might be possible in the unit test.

In D80531#2284388, @kkleine wrote:

Hi @dblaikie . I did run ninja check-all and /bin/llvm-lit -av ../clang-tools-extra/test/clang-tidy/checkers/modernize-replace-disallow-copy-and-assign-macro.cpp on this very new revision (abd70fb3983f342bc1c90f9c70a7b59790ad5206) manually but I don't see this error coming up. Can you please try to test if you see the error on a fresh build?

In D87673#2290926, @sammccall wrote:

In D87673#2290838, @dblaikie wrote:

In D87673#2275940, @sammccall wrote:

Thanks, this seems better than crashing.
The practical result isn't wonderful: the two are going to fight over index files to some extent. But this can happen with different clangd versions (that use different index format versions) anyway. So this is really just graceful recovery from a bad situation.

In D87673#2273347, @ArcsinX wrote:

Unsure about test for this

Agree, testing this seems hard and not that useful.

FWIW here's how to test codepaths only reachable when zlib is not available:

./llvm/test/tools/llvm-dwp/X86/nocompress.test:

REQUIRES: !zlib

Yep, that requires a checked-in binary file, which in the case of that test is in a standard stable format, which this data is not, so we'd also need tools to regenerate it. And we don't have APIs to generate the file without compression if zlib is available (because we never want to do that), so we'd need to add those or require anyone touching the file format to build a no-zlib tree to regenerate the file.
None of this is impossible, but this is pretty unlikely to catch a bug, and it's very unlikely to be a high-impact bug..

In D87673#2275940, @sammccall wrote:

Thanks, this seems better than crashing.
The practical result isn't wonderful: the two are going to fight over index files to some extent. But this can happen with different clangd versions (that use different index format versions) anyway. So this is really just graceful recovery from a bad situation.

In D87673#2273347, @ArcsinX wrote:

Unsure about test for this

Agree, testing this seems hard and not that useful.

Generally it's handy to include some information about the reason for the revert in the commit message so others can follow along (makes it easier to know if a specific revert will address a breakage someone else might be seeing, etc - also useful in future review/recommit/work in this area, so there's some history of what sort of things can/did go wrong) - links to or summaries of buildbot failures, etc, are good.

In D87808#2286664, @rsmith wrote:

In D87808#2280197, @dblaikie wrote:

@rsmith What's the deal with these anonymous structs/unions? Why do they have copy/move constructors (are those technically called from the enclosing class's copy/move constructors?) but no default constructor to be called from the other ctors of the enclosing class?

Let's use the class G in the example. G::G directly initializes the field g_, so it can't call any kind of constructor for the anonymous struct member. Instead, anonymous structs and unions are "expanded" when building the constructor initializer lists for all cases other than a copy or move constructor (no initialization action is taken for union members by default, though -- not unless they have a default member initializer or an explicit mem-initializer). So the default constructor of an anonymous struct or union is never really used for anything[*].

But now consider the copy or move constructor for a type like this:

struct X {
  Y y;
  union { ... };
};

That copy or move constructor needs to build a member initializer list that (a) calls the Y copy/move constructor for y, and (b) performs a bytewise copy for the anonymous union. We can't express this in the "expanded" form, because we wouldn't know which union member to initialize.

So for the non-copy/move case, we build CXXCtorInitializers for expanded members, and in the copy/move case, we build CXXCtorInitializers calling the copy/move constructors for the anonymous structs/unions themselves.

In D68620#2287669, @Orlando wrote:

Just to clarify: when I say "elf" here I'm talking about the linked executable file, and "object files" are the pre-link .o files.

In D87147#2288024, @aprantl wrote:

I think the comment in https://reviews.llvm.org/rL349065 describes it quite well:

The DIFile used by the CU is special and distinct from the main source
file. Its directory part specifies what becomes the DW_AT_comp_dir
(the compilation directory), even if the source file was specified
with an absolute path.

So while the CU links to a DIFile it's more a storage mechanism for DW_AT_comp_dir and not a "file" in the normal sense and thus should be treated special. (And we might want to just store a string instead to avoid this confusion in the future.

sounds good - thanks!

maybe slightly long test case - could do this with the CU DIE itself? (perhaps a case where hand-crafted/substantially hand-modified assembly is OK, because it can be made /so/ simple - or maybe yaml2obj dwarf support is adequate for this use case?)

Looks good - thanks!

does this classify as a warning? Is there a distinction between ill-formed DWARF and "this is valid, but unlikely to be great"?

Thanks for all the data @Orlando - do you happen to have means to measure total object size too? Be useful to compare the binary size increase with the object size decrease.

In D87935#2284630, @jhenderson wrote:

My instinct is that I prefer a third approach - simply keep both combined and separate lists - the combined list (could be a vector or an ordered map) is the offset-ordered list of all units, whilst there are then separate lists for compile and type units, with members being pointers to the units stored in the primary list. I think this has the advantage of being very explicit about things, with the drawback being that there are multiple members to keep in sync (possibly alleviated by having a wrapper class around the containers).

In rG0526713aa8017bc0dc8f6d0dd14238ef788400b9#948086, @MaskRay wrote:

Its two callers has excluded the possibility that
F.doesNotRecurse(), so this is safe.

Seems like this change would end up throwing off the NumNoRecurse statistic? and changes the behavior for callers (looks like it bubbles up to the "Changed" feature of passes, which means this change might cause analysis invalidation even when the IR was not changed (because the function was already marked norecurse)?

In D87837#2283784, @chandlerc wrote:

Good to confirm with Dave of course, but this looks pretty great to me, and thanks so much!

Rather than "non-word" maybe "path" would be more legible (that's the terminology the message text uses, and seems shorter/clearer in the warning name/constant name/etc), ie: -Wuse-ld-path

@rsmith What's the deal with these anonymous structs/unions? Why do they have copy/move constructors (are those technically called from the enclosing class's copy/move constructors?) but no default constructor to be called from the other ctors of the enclosing class?

In D68620#2279460, @probinson wrote:

In D68620#2276034, @Orlando wrote:

(as an aside: do you/@probinson have much of a sense of how much binary size increase you'd trade for object size reductions in this space? If binary size is ulmtimately what you care most about, I'm guessing maybe debug_loc base address specifiers will never be a win for you & perhaps we should just group them under a flag, maybe refactor the debug_ranges base address specifier flag to cover both (the flag there was introduced due to a gold+gdb_index+32 bit binary bug, unfortunately, but lumping them together seems OK-ish to me))

I'm not sure, I'll defer to @probinson on that.

What we actually care about is turnaround time, which for debug info encompasses compile time, link time, debugger startup time, and the attendant I/O latencies. Note that console download time is *not* a factor, as our downloader knows to omit the debug info.

It's not exactly raw size that matters, but we ought to care how efficiently the information is encoded in the file data. So, with respect to location lists, multiplying the number of entries without reducing the number of relocations is bad all around.

Could you include the source and any command lines (clang, what optimization level, etc) was used to generate the test IR? It can make the test a bit easier to read at a glance and help if the test needs to be regenerated for some reason.

In D86847#2278056, @MaskRay wrote:

The idea is to add a terminator block, but a terminator carries less information so this patch intends to add a size block instead. With a terminator, the following problem can be better addressed

// We may be consuming bitcode from a client that leaves garbage at the end
// of the bitcode stream (e.g. Apple's ar tool). If we are close enough to
// the end that there cannot possibly be another module, stop looking.

If the container does not expect more than one bitcode stream. Then after passing one stream, it can bail out.
For users who want to have multiple streams, they can strip any padding. The scheme will automatically work with most binary formats (alignment=1).

In D87147#2276547, @umesh.kalappa0 wrote:

In D87147#2275685, @dblaikie wrote:

Hmm, sorry for the churn here - I was going to commit this, but looking at it, it seems this patch pretty much exactly undoes the work from rL349065 - @aprantl can you weigh in on this? This patch doesn't seem to break your test - did your test test the issue correctly? Maybe some other change that's happened since your change has made reverting it viable while preserving the desired behavior?

thank you @dblaikie for commiting this.

Looks good - thanks!

In D68620#2276034, @Orlando wrote:

Thanks for the data!

Happy to help :)

(as an aside: do you/@probinson have much of a sense of how much binary size increase you'd trade for object size reductions in this space? If binary size is ulmtimately what you care most about, I'm guessing maybe debug_loc base address specifiers will never be a win for you & perhaps we should just group them under a flag, maybe refactor the debug_ranges base address specifier flag to cover both (the flag there was introduced due to a gold+gdb_index+32 bit binary bug, unfortunately, but lumping them together seems OK-ish to me))

I'm not sure, I'll defer to @probinson on that.

When you say "llvm-master before/after your commits" - what version of llvm-master and what commits did you have to test with? (if you can/want to discuss them)

I'm rather surprised, if master was moderately recent, that it shows no benefit from https://reviews.llvm.org/rG57d8acac64b87cb4286b00485fb2da7521fc091e (perhaps, if it's not too much hassle, you could run a sample benchmark before/after that change?)

Apologies for being unclear. Stats for "before my commits" are taken from a build at the first commit before D79571, and "with my commits" is with D86153 / rG57d8acac64b87cb4286b00485fb2da7521fc091e applied (and all the commits in between, including D82129).

I don't fundamentally mind not supporting this - especially with asan checks that might help catch clients in the codebase that violate the contract. But I'm guessing there's existing violations in the codebase given the previous patches/filed bugs about implementing this behavior.

Hmm, sorry for the churn here - I was going to commit this, but looking at it, it seems this patch pretty much exactly undoes the work from rL349065 - @aprantl can you weigh in on this? This patch doesn't seem to break your test - did your test test the issue correctly? Maybe some other change that's happened since your change has made reverting it viable while preserving the desired behavior?

In D87641#2275598, @aprantl wrote:

I'm expecting this to cause all sorts of non-clang frontends to trigger this assertion, since we don't require types to have sizes. In fact, I have an open bug for the Swift frontend to try removing the static sizes from types, since LLDB prefers the dynamic runtime size anyway.
If we want to make this a requirement, we should probably implement it as a verifier check instead of an assertion. But I'm not sure if we want to require this for all frontends.

Hmm - probably simpler to justify this patch by including refactoring at least one (possibly more) existing uses you mentioned, to use this API - then I probably don't really need to understand what it's doing, just that it's generalizing some existing functionality.

In D87326#2273410, @dblaikie wrote:

In D87326#2273355, @nikic wrote:

Tested the newest version of this patch and I'm still seeing massive regressions, even larger than before.

Compile-time (instructions retired): https://llvm-compile-time-tracker.com/compare.php?from=cc947207283f934c72af0eb0b1a08978c59d40a2&to=d99c6d441764431519c1c11d490e7e88ffe06775&stat=instructions This is now a 1.4% geomean regression at O3, with 2% at O3, with tramp3d-v4 hitting 3%.

Max RSS: https://llvm-compile-time-tracker.com/compare.php?from=cc947207283f934c72af0eb0b1a08978c59d40a2&to=d99c6d441764431519c1c11d490e7e88ffe06775&stat=max-rss This is now a 1.6% geomean regression at O3, with 2.1% at O0.

Clang text size goes from 80560905 to 82179908, a 2% regression (non-LTO build using GCC 9.3).

@njames93 - mind looking at the memory usage a bit? that's probably relatively easy to check (well, maybe not, since it'll be smeared over all the different instantiations of this template) & seems a bit surprising. Code growth is probably just what it is - but if there's a way to quantify it and check we've got minimal extra instantiations, that'd be good.

The compile-time stat: @nikic: can you measure a wall performance difference? (otherwise possible they're shorter instructions, etc/ doesn't necessarily mean this makes compile times worse, perhaps?)

In D68620#2274165, @Orlando wrote:

@Orlando mentioned he was collecting some size data that would be relevant here, he'll post it when he's done. Basically .debug_loc sizes at various points.

Hi!

I've built a benchmark suite of 85 programs with "-O2 -g" with our downstream branch targeting X86 emitting DWARF v4.

This table provides a summary of the data. It shows the mean for some size data normalized as a percentage of the llvm-3 results for each benchmark.

For reference:
Largest binary built with llvm-3: 6010 kB
Smallest binary built with llvm-3: 79 kB

+------------------------------------------------------------------------------------------- +
| Mean binary size for benchmarks normalized as a percentage of llvm-3 builds                |
+---------------------------------+------------+------------------+-----------------+--------+
| llvm version                    | .debug_loc | other debug info | everything else | Total  |
+---------------------------------+------------+------------------+-----------------+--------+
| llvm-3                          | 13.7       | 33.2             | 53.1            | 100    |
| llvm-4                          | 12.7       | 33.8             | 53.8            | 100.3  |
| llvm-5                          | 13.4       | 35.6             | 54.6            | 103.7  |
| llvm-7                          | 18.4       | 35.6             | 54.0            | 108.0  |
| llvm-8                          | 17.5       | 37.1             | 54.5            | 109.1  |
| llvm-9                          | 19.7       | 37.2             | 54.6            | 111.5  |
| llvm-10 before dblaikie commit  | 19.8       | 37.4             | 54.9            | 112.1  |
| llvm-10 with dblaikie commit    | 25.6       | 37.4             | 54.9            | 117.9  |
| llvm-10                         | 25.8       | 37.5             | 54.8            | 118.1  |
| llvm-master before my commits   | 26.2       | 37.4             | 54.8            | 118.4  |
| llvm-master with my commits     | 18.4       | 35.5             | 55.3            | 109.3  |
+---------------------------------+------------+------------------+-----------------+--------+

Here's an image of that data in graph form: M3.

The llvm-6 entry has been omitted because the non debug-info size is a distracting outlier. The .debug_loc section size is ~16.5% for that one.

If you'd like to see the data in another format or see data for the benchmarks individually please let me know.

In D87326#2273355, @nikic wrote:

Tested the newest version of this patch and I'm still seeing massive regressions, even larger than before.

Compile-time (instructions retired): https://llvm-compile-time-tracker.com/compare.php?from=cc947207283f934c72af0eb0b1a08978c59d40a2&to=d99c6d441764431519c1c11d490e7e88ffe06775&stat=instructions This is now a 1.4% geomean regression at O3, with 2% at O3, with tramp3d-v4 hitting 3%.

Max RSS: https://llvm-compile-time-tracker.com/compare.php?from=cc947207283f934c72af0eb0b1a08978c59d40a2&to=d99c6d441764431519c1c11d490e7e88ffe06775&stat=max-rss This is now a 1.6% geomean regression at O3, with 2.1% at O0.

Clang text size goes from 80560905 to 82179908, a 2% regression (non-LTO build using GCC 9.3).

Great, thanks a bunch!

In D87326#2272934, @njames93 wrote:

In D87326#2272611, @dblaikie wrote:

Use global namespace new inside GrowBuffer.

Why this change? Would've thought op new overloads might be desirable, but I certainly don't know the specifics off-hand right now.

SmallVector seems to use global namespace new for all its operations, so I thought I'd follow the convention there.

In D87657#2272901, @phosek wrote:

So a trivial example is:

#include <string>
int main() {}

When compiled as:

clang++ test.cc -g3 -S -o test.S

I see paths like these in the output:

"/usr/local/google/home/phosek/fuchsia" "prebuilt/third_party/clang/linux-x64/bin/../include/c++/v1/cstddef"

We could go over driver code and make sure we always invoke sys::path::remove_dots inside methods like AddIncludePaths or addPathIfExists (see for example https://github.com/llvm/llvm-project/blob/b3afad046301d8bb1f4471aceaad704b87de3a69/clang/lib/Driver/ToolChains/Gnu.cpp#L2902 which where the .. in libc++ header path comes from), but maybe it'd be better to do the normalization uniformly when emitting debuginfo metadata?

Sounds alright - few things could be simplified, etc.

Got any particular examples of this from LLVM's DWARF output? I suspect this sort of thing might come from/lead to path non-canonicalization which would make the line table bigger than it needs to be, so might be worth fixing there too.

Is this testable? It should produce fewer element copies, yes? Could you add a test for it?

then when the call to insert etc is made the container grows a second time. In that case the element to insert would now be in freed memory which would cause asan to fail. However right now that test case doesn't seem to appear, it does happen where the container grows from small size to insert. but as the memory is stack based there, its fine, from asans POV, to reference it.
In D87237, I experimented by putting asan instrumentation inside SmallVector, which caused a test case to fail when ran under asan.

OK, so I think you're saying - the current in-tree tests, and the tests you're adding, currently don't fail even though they do test self-insertion only because they're testing in small -> big (not big -> big) mode? But with ASan instrumented SmallVector, even a small->big would cause test failures of the existing tests, without this patch to fix self-insertion?

The tests inside Constructable will cause the current in-tree self-insertion test to fail as that will detect copying from a destructed value in the call to insert. The asan instrumented SmallVector would also fail because it will detect accessing the poisoned inline buffer in the small->big growth. However with this patch both of these causes of failing tests will be fixed.

I'll leave this one to @aprantl - though I am a bit concerned about whether this should be a verifier error if SROA is going to assume it's an invariant. Some discussion on the bug about whether this SROA code is only reachable for some subset of types that do have sizes, while some other types won't have sizes. (variable length arrays being a fairly fundamental example, since they can't have a known length - but I would worry that SROA might be able to mess with known portions of them, maybe?) I mean, I'm usually all for "throw an assert in, and if it ever fails then we'll have figured out what case we didn't consider earlier", though just a bit hesitant on this at the moment. Probably no big deal, though.

In D87560#2269528, @Paul-C-Anagnostopoulos wrote:

The patch has been pushed.

In D68620#2271269, @probinson wrote:

In D68620#2266531, @dblaikie wrote:

@probinson - do you have any state here? Whether this is an ongoing issue, whether the improvements to avoid unnecessary debug_loc have reduced the overhead sufficiently, etc?

@Orlando mentioned he was collecting some size data that would be relevant here, he'll post it when he's done. Basically .debug_loc sizes at various points.

Couple of things to clean up in the test, but otherwise looks good. Appreciate your patience with all the fussy details.

Can the test be reduced down to "extern int i; int i;" now?

In D87326#2270142, @njames93 wrote:

In D87326#2269796, @MaskRay wrote:

Yeah, seems that SmallVector::push_back should be fixed to allow a.push_back(a[0]). SmallVector::insert has been fixed in rL134554

It hasn't, that handles the case where the element to insert is moved when shifting the elements down to make room, but not when the element to insert is moved because of container growth.

In D87326#2269796, @MaskRay wrote:

Yeah, seems that SmallVector::push_back should be fixed to allow a.push_back(a[0]). SmallVector::insert has been fixed in rL134554

Ah, right, sort of a related but maybe slightly different problem - when not resizing the underlying buffer, elements are moved, then the new one is inserted so the reference may've been invalidated. Probably best to handle that in a separate/follow-up commit - any idea if the C++ standard guarantees this for std::vector, for instance? And if so, how does libc++ implement this guarantee?

In D87326#2269455, @njames93 wrote:

In D87326#2269286, @dblaikie wrote:

Hmm - I think maybe what I had trouble with was understanding how much this shuold generalize. Should this handle subranges? (if you try to append half of a vector onto itself) and if so, how? Anyone happen to know what's guaranteed by the standard (perhaps only push_back of single elements - that's easier to handle)

From what I can see insert and append fully support when the range to insert is enclosed by the vector, however assign makes no such promise as it would potentially require allocating more storage even if the container itself doesn't need to grow

In D87147#2269369, @umesh.kalappa0 wrote:

extern int i;
int i;

We are not getting 2 DIFile entries for the above case,for your reference .

!0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
!1 = distinct !DIGlobalVariable(name: "i", scope: !2, file: !3, line: 2, type: !6, isLocal: false, isDefinition: true)
!2 = distinct !DICompileUnit(language: DW_LANG_C99, file: !3, producer: "clang version 10.0.0.1 (http://ukallapa@stash.wrs.com/scm/llvm/clang.git 2660105769d58dbb8b66f56247f208ca2872913a) (llvm/llvm.git bfc5adc611fdc055ece9191c3d8df0750bec8816)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !4, globals: !5, splitDebugInlining: false, nameTableKind: None)
!3 = !DIFile(filename: "test.c", directory: "/folk/ukallapa")
!4 = !{}
!5 = !{!0}
!6 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
!7 = !{i32 7, !"Dwarf Version", i32 4}
!8 = !{i32 2, !"Debug Info Version", i32 3}
!9 = !{i32 1, !"wchar_size", i32 4}
!10 = !{!"clang version 10.0.0.1 (http://

I believe I filed the second bug ( https://bugs.llvm.org/show_bug.cgi?id=16253 ) a while back because I tried to do it myself (admittedly, it was myself 7 years ago, so maybe my perspective has changed over the years) and found it too difficult. That there were corner cases or something that made me unsure how to move forward - so I filed the bug and forgot about it.

Could this be done with a custom matcher, perhaps? (might be a bit tidier than macros with FILE and LINE manually handled, etc?

Sounds good to me

In D87009#2267663, @ikudrin wrote:

In D87009#2266648, @dblaikie wrote:

Let's take, for example, DIEExpr. As far as I can find, with DW_FORM_sec_offset it is used only in the DebugInfo/DWARF unit tests, in dwarfgen::DIE::addStrOffsetsBaseAttribute(). If we took the approach with end-to-end functional testing, this class would not be updated. And the path is not simple to track, by the way.

API only intended for unit tests would be tested/updated with unit tests.

The DIEExpr class is not intended to be used only in unit tests; the CodeGen library uses it, but with different DWARF forms. The generator in DebugInfo/DWARF unit tests does not test the class, it just uses it to create some bits of DWARF data. Moreover, these unit tests are for a different library and as such are not the best place to be updated if we want to fix the issue in a class of the CodeGen library. It is much better to test the implementation in the library's own unit tests, so this patch does exactly that. I do not want to leave DIEExpr in the not updated state, because the new code may use it without noticing that the implementation is broken for DWARF64.

In D87021#2267680, @ikudrin wrote:

In D87021#2267059, @dblaikie wrote:

The test sources were based on some simple code, but then they were cleared as much as I could.

You mean the IR was modified after compilation? Or that the original C source was simplified as much as possible?

I removed/adjusted some attributes here and there to make the tests smaller and to express mostly what I wanted to test in each particular case. In some cases, like the test for .debug_macro, the source was created fully manually, based on existing tests.

Usually we don't simplify the IR - not worth it/doesn't make things especially more maintainable & more likely to introduce something that makes the IR quirky/not representative of the normal Clang generated IR.

I'll see if I can update the tests in the series so that the IR is fully generated by clang.

Looks good - thanks!

Looks good - thanks!

Looks good, thanks!

In D87159#2256991, @csigg wrote:

Hi David, is it OK if I send these changes your way for review? There is maybe one or two more after that for now.

This revision primarily adds some helpers to skip printing wrapper types through inheritance or composition.

Thanks!