In D113741#3538747, @dblaikie wrote:

Right, so this does only reproduce with Split DWARF + Split DWARF inlining + ThinLTO, I think. The CUs for the imported units Split DWARF inlining are being designated as split units, but then split units aren't actually emitted (because of the single-unit limitation of Split DWARF) and so the pubnames are emitted for DIEs that were never layed out/emitted, by the looks of it, so they get an offset of zero, which looks like/creates a truncated pubnames contribution.

Trying to make a small reproducer.

In D126257#3539510, @DavidSpickett wrote:

In https://reviews.llvm.org/rG38eb4fe74b38 I moved the generic test into the X86 folder as it uses an X86 triple. You could think of the folder names there having 2 meanings, 1: the tests look at things specific to that arch and 2: they use that arch's triple (I think X86 is usually the one people choose if they want to test something generic anyway).

In D124892#3532246, @ayermolo wrote:

In D124892#3529768, @dblaikie wrote:

With ThinLTO enabled we can have a function in a CU that doesn't have split dwarf inlining enabled that gets inlined into CU that has it enabled.
When we try to lookup Abstract Scope Die for the inlined scope in constructInlinedScopeDIE, it's not present and we assert.

So it seems there's probably some disconnect between the logic that adds abstract scope DIEs and the logic that looks them up? (one's using the source CU, one's using the destination CU?) - can you confirm that/which one's doing which?

& maybe them consistent would be good/suitable - I guess the right behavior is probably to treat the non-split-dwarf-inlining function as having no debug info for the purposes of the place it's inlined into?

So using this example.
When we are processing MachineFunction for main. We are processing SubProgram main, with CU in main.dwo. In constructAbstractSubprogramScopeDIE this is the SrcCU. It has debug inlining enabled.
What gets inlined in to it is Abstract Lexical Scope from helper.dwo that doesn't have debug inlining enabled.
Because of it we construct Abstract Subprogram Scope DIE only in SrcCU (Main DWO CU).

With my "fix" what happens is we end up creating CU for the helper dwo, Skeleton CU for it, and adding inline subprogram information to it for "func".
So when we construct subprogram scope die, when constructInlinedScopeDIE is invoked on scope for func (which comes from helper.cpp), we are able to find Inlined sub program and don't crash. Because we can find Origin DIE for inlined sub program.

Ideally I think we would like to have inlined sub program in main Skeleton CU. Otherwise it won't be complete.

Right, so this does only reproduce with Split DWARF + Split DWARF inlining + ThinLTO, I think. The CUs for the imported units Split DWARF inlining are being designated as split units, but then split units aren't actually emitted (because of the single-unit limitation of Split DWARF) and so the pubnames are emitted for DIEs that were never layed out/emitted, by the looks of it, so they get an offset of zero, which looks like/creates a truncated pubnames contribution.

Abandoning this since the discussion ended up pointing towards a preference for a distinct noipa attribute. Then that stalled out in discussions about whether that could be implemented via isInterposable or would need a separate proprety and then revisiting/restesting every check for interposability to instead test "interposable or noipa", basically... which made me a bit exhausted to think about visiting/changing/retesting all those cases and so I haven't picked this up again. It's over in D101011 if anyone wants to weigh in there/pick it up/poke it with a stick, etc.

Sorry, with all the layers in the previous messages I'm not sure what the summary is. Sounds like the summary is "this is OK to continue work in the direction of supporting [[clang::warn_unused_result]] (only that spelling) for typedefs in C and C++ (for compatibility with C/so that C headers, like LLVM C's APIs can still be parsed as C++), while considering some of the complications raised in the WG21 discussion"?

Separate out clang:warn_unused_result so typedef support can be added to only that spelling.

In D113741#3538443, @dblaikie wrote:

OK, so if I want to reproduce the issue for you, I need to apply these three patches? D114705 D113741 D113743

OK, so if I want to reproduce the issue for you, I need to apply these three patches? D114705 D113741 D113743

In D122974#3536342, @llunak wrote:

In D122974#3535669, @dblaikie wrote:

It doesn't make sense to require a stable hash algorithm for an internal cache file.

It's at least a stronger stability requirement than may be provided before - like: stable across process boundaries, at least, by the sounds of it? yeah?

It's meant to be the same for the same library binary.

& then still raises the question for me what about minor version updates, is it expected to be stable across those?

Is anything expected to be stable across those? If so, that doesn't seem to be the reality of it (a random quick check finds two ABI changes just between 14.0.4 and 14.0.5, e6de9ed37308e46560243229dd78e84542f37ead and 53433dd0b5034681e1866e74651e8527a29e9364).

In D126257#3538331, @pcwalton wrote:

Oh, what I mean is that our choices in the case in which zero-sized global symbols are forbidden at the IR level would be (1) don't emit source-level zero-sized symbols into LLVM IR (or don't emit DWARF metadata for them), or (2) round all zero-sized symbols up to one byte. If we pick (1), I don't think they would show up in the debug info, which could be confusing for programmers. If we pick (2), then certain abstractions which are zero-cost today in Rust become non-zero-cost.

Rebase

Sounds good - maybe add some static_asserts in the unit test to validate that these operations are appropriately deleted? & could you make the comment more definitive - "Delete these operations as the defaults would be shallow/cause incorrect shared state. Implement these if/when needed/worthwhile to do so." or something like that.

Looks good - thanks for your patience & considering alternatives/helping me understand what was going on here.

In D113741#3534761, @ayermolo wrote:

In D113741#3532850, @dblaikie wrote:

In D113741#3516038, @krisb wrote:

There is an alternative implementation that relies on addition field of DISubprogram/DILexicalScope that tracks static locals, local imports and types D125693.

Ah, OK - so that 5-part sequence is an alternative to this one?

Might be good to figure out the issue here before debating which direction to go in, etc. (hmm, one of the patches in that series I've already approved? So am I reviewing alternatives here? Given the complexity here that's a bit costly for me/end up feeling like I'm drowning under trying to figure out all these moving parts... - a linear sequence is generally OK/good, I can review the easy parts and focus on the difficult ones, but multiple large alternatives can be a bit daunting)

In D126257#3535908, @pcwalton wrote:

Thanks!

Wouldn't that mean that those zero-sized data symbols won't show up in the debugger anymore? That might be confusing for Rust users, who do use zero-sized globals reasonably commonly (to attach methods to).

In D102122#3344317, @dblaikie wrote:

I don't recall all the context, but did try discussing this with the committee folks and got a variety of strong opinions/wasn't sure whether there was a path forward: https://lists.isocpp.org/ext/2021/05/index.php#msg16554 (for those with access to that). What's your take on the discussion there? Worth pushing forward?

Ping

Rebase

rebase

@rsmith I had a few responses to your last round of review here - could you have a look through them/see if this is the right way to go, or if more refactoring would be suitable?

In D126257#3535829, @pcwalton wrote:

How's this?

It doesn't make sense to require a stable hash algorithm for an internal cache file.

If you want to add any of the history, looks like this is my first email proposing the direction: https://lists.llvm.org/pipermail/llvm-dev/2015-February/081822.html

Looking at this further, if we were going to add a copy, it'd be good for it to be more efficient & probably suitable to have an efficient move operation too - but that's all a bit complicated/bigger project than I think we should dive into right now - so let's go with the D125611 solution for now.

Looking at D125899 in more detail I'm OK with deciding that adding good quality copy/move semantics to IntervalMap is a bigger project than I'm up for right now & so let's go with this direction instead (sorry for the back and forth - thanks for understanding).

In D126010#3529743, @dblaikie wrote:

In D126010#3528129, @rnk wrote:

I agree with David, I would like to see LLVM move in the direction of never emitting empty functions. These are just labels that snap to the next function in the same section, and that's silly. I'm not sure what happens if you use function sections. We should just emit some trap instruction, and let the linker do identical code folding (ICF) to merge them back together. This will regress code size, but I doubt out users will complain, and ICF will recover most of the size regression.

Regarding global symbols, I don't know about Rust, but I believe it is possible to emit empty global variables in LLVM IR with zero-sized arrays. So, I think this change probably has merit on its own, without getting into the handling of empty function bodies.

LLVM IR/C++ does support zero-sized arrays, though I think even in that case it might be better to make them non-zero symbols for symbolizing purposes, etc - GCC seems to make them non-zero size: https://godbolt.org/z/vh4z3G4fh

In D113741#3516038, @krisb wrote:

There is an alternative implementation that relies on addition field of DISubprogram/DILexicalScope that tracks static locals, local imports and types D125693.

In D126059#3532264, @clayborg wrote:

A 1 to 2 percent improvement is probably going to go away if the asserts are enabled when it has to do the check each time.

Any chance of a template to avoid duplicating this code between case sensitive and case insensitive - looks like a long enough function with room for bugs/fixes/changes that it'd be worth avoiding duplication?

In D122974#3481269, @llunak wrote:

In D122974#3480686, @dblaikie wrote:

In D122974#3424654, @JDevlieghere wrote:

...

struct HashedStringRef {
  const llvm::StringRef str;
  const unsigned full_hash;
  const uint8_t hash; 
  HashedStringRef(llvm::StringRef s) : str(s), full_hash(djbHash(str)), hash(hash(str)) {}
}

...

The external code shouldn't be able to create their own (ctor private/protected, etc) - the only way to get one is to call StringMap to produce one.

But what if I don't want StringMap to compute the hash at all? There's still that 10-15% of CPU time spent in djbHash() when debug info uses exactly[*] that (and LLDB's index cache could be modified to store it). Which means it can be useful for StringMap to allow accepting precomputed hash, and then what purpose will that HashedStringRef have?

Ping

In D126224#3531949, @rnk wrote:

I am reminded of the perennial problem of "optional" protobuf fields that, when omitted, will cause production crashes.

Do you think it would be less disruptive to synthesize a name? I believe the string lives in a string pool, so naming all string literals <stringliteral> seems like it could be acceptable.

In D125979#3532163, @avl wrote:

In D125979#3529745, @dblaikie wrote:

(any chance we can avoid duplicating this, and instead refactor the lld one into LLVM and have lld use that one instead of using its own local copy?)

I thought about that. But the implementation from lld looks too specific for general implementation.
It uses knowledge about stored data:

1. It keeps data together with hash inside hash table. This is OK for 32-bit integer but it would not be OK for larger type.

With ThinLTO enabled we can have a function in a CU that doesn't have split dwarf inlining enabled that gets inlined into CU that has it enabled.
When we try to lookup Abstract Scope Die for the inlined scope in constructInlinedScopeDIE, it's not present and we assert.

You mention performance concerns - even in an optimized build? What sort of performance data do you have? Would be good to have that documented here in the review/in the commit message to justify the use of these more error-prone APIs.

Seems OK

Might be worth a note in the Release Notes given that it's at least caused issues with one DWARF consumer.

Test coverage? (possibly just modifying much of the existing testing to occur in a constexpr context, if possible)

(any chance we can avoid duplicating this, and instead refactor the lld one into LLVM and have lld use that one instead of using its own local copy?)

In D126010#3528129, @rnk wrote:

I agree with David, I would like to see LLVM move in the direction of never emitting empty functions. These are just labels that snap to the next function in the same section, and that's silly. I'm not sure what happens if you use function sections. We should just emit some trap instruction, and let the linker do identical code folding (ICF) to merge them back together. This will regress code size, but I doubt out users will complain, and ICF will recover most of the size regression.

Regarding global symbols, I don't know about Rust, but I believe it is possible to emit empty global variables in LLVM IR with zero-sized arrays. So, I think this change probably has merit on its own, without getting into the handling of empty function bodies.

I think the right solution to this is not to have zero sized symbols at all (they break C++ function pointer uniqueness, make symbolising "weird"/ambiguous, break aranges here, and if they are functions and those functions are called they result in some pretty arbitrary badness as execution falls off - and we should probably fix that in general so you can't just fall off the end of a function, the code size wins aren't enough to justify it I believe/think/would be good to have some data to back that up)

In D125899#3522879, @kparzysz wrote:

Changed the copy to a deep one.

Looks OK otherwise

Awesome, thanks!

In D125611#3519571, @dim wrote:

In D125611#3517574, @dblaikie wrote:

Some more details on the specifics in the patch description would be good - I'm still not 100% clear on the reproduction case to understand what we're working around, exactly. Do you have a godbolt reproduction/small example, by chance?

Assuming there's a directory with llvm includes, this is enough to reproduce:

#include "llvm/ADT/IntervalMap.h"
#include <vector>

void f(std::size_t size)
{
  using namespace llvm;

  IntervalMap<uint32_t, uint64_t>::Allocator Alloc;
  std::vector<IntervalMap<uint32_t, uint64_t>> Sections(size, IntervalMap<uint32_t, uint64_t>(Alloc));
}

which will (on FreeBSD at least, where libc++ is the standard C++ library) result in something like:

$ clang++ -I llvm/include -c intervalmap.cpp
In file included from intervalmap.cpp:1:
In file included from llvm/include/llvm/ADT/IntervalMap.h:108:
In file included from llvm/include/llvm/ADT/SmallVector.h:19:
In file included from /usr/include/c++/v1/algorithm:643:
/usr/include/c++/v1/memory:1809:31: error: call to implicitly-deleted copy constructor of 'llvm::IntervalMap<unsigned int, unsigned long, 12, llvm::IntervalMapInfo<unsigned int>>'
            ::new((void*)__p) _Up(_VSTD::forward<_Args>(__args)...);
                              ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/c++/v1/memory:1687:21: note: in instantiation of function template specialization 'std::__1::allocator<llvm::IntervalMap<unsigned int, unsigned long, 12, llvm::IntervalMapInfo<unsigned int>>>::construct<llvm::IntervalMap<unsigned int, unsigned long, 12, llvm::IntervalMapInfo<unsigned int>>, const llvm::IntervalMap<unsigned int, unsigned long, 12, llvm::IntervalMapInfo<unsigned int>> &>' requested here
                __a.construct(__p, _VSTD::forward<_Args>(__args)...);
                    ^
/usr/include/c++/v1/memory:1538:14: note: in instantiation of function template specialization 'std::__1::allocator_traits<std::__1::allocator<llvm::IntervalMap<unsigned int, unsigned long, 12, llvm::IntervalMapInfo<unsigned int>>>>::__construct<llvm::IntervalMap<unsigned int, unsigned long, 12, llvm::IntervalMapInfo<unsigned int>>, const llvm::IntervalMap<unsigned int, unsigned long, 12, llvm::IntervalMapInfo<unsigned int>> &>' requested here
            {__construct(__has_construct<allocator_type, _Tp*, _Args...>(),
             ^
/usr/include/c++/v1/vector:1066:25: note: in instantiation of function template specialization 'std::__1::allocator_traits<std::__1::allocator<llvm::IntervalMap<unsigned int, unsigned long, 12, llvm::IntervalMapInfo<unsigned int>>>>::construct<llvm::IntervalMap<unsigned int, unsigned long, 12, llvm::IntervalMapInfo<unsigned int>>, const llvm::IntervalMap<unsigned int, unsigned long, 12, llvm::IntervalMapInfo<unsigned int>> &>' requested here
        __alloc_traits::construct(this->__alloc(), _VSTD::__to_address(__pos), __x);
                        ^
/usr/include/c++/v1/vector:1159:9: note: in instantiation of member function 'std::__1::vector<llvm::IntervalMap<unsigned int, unsigned long, 12, llvm::IntervalMapInfo<unsigned int>>, std::__1::allocator<llvm::IntervalMap<unsigned int, unsigned long, 12, llvm::IntervalMapInfo<unsigned int>>>>::__construct_at_end' requested here
        __construct_at_end(__n, __x);
        ^
intervalmap.cpp:9:48: note: in instantiation of member function 'std::__1::vector<llvm::IntervalMap<unsigned int, unsigned long, 12, llvm::IntervalMapInfo<unsigned int>>, std::__1::allocator<llvm::IntervalMap<unsigned int, unsigned long, 12, llvm::IntervalMapInfo<unsigned int>>>>::vector' requested here
  std::vector<IntervalMap<uint32_t, uint64_t>> Sections(size, IntervalMap<uint32_t, uint64_t>(Alloc));
                                               ^
llvm/include/llvm/ADT/IntervalMap.h:970:14: note: copy constructor of 'IntervalMap<unsigned int, unsigned long, 12, llvm::IntervalMapInfo<unsigned int>>' is implicitly deleted because variant field 'leaf' has a non-trivial copy constructor
    RootLeaf leaf;
             ^
1 error generated.

Ultimately, this is caused by _LIBCPP_DEPRECATED_ABI_DISABLE_PAIR_TRIVIAL_COPY_CTOR in /usr/include/c++/v1/__config (this was last modified by @EricWF, but a long time ago). If this setting is enabled (to preserve the old ABI), std::pair will always have a non-trivial copy constructor, and since IntervalMap contains a std::pair (via RootLeaf -> IntervalMapImpl::LeafNode -> NodeBase<std::pair<KeyT, KeyT>, ValT, N>), IntervalMap itself is also non-trivially copyable. std::vector in turn requires its value type to be copy-constructible (at least certainly when you use the constructor with a size).

In D123319#3506745, @shafik wrote:

Any update/further details here?

David, apologies for not getting back to you sooner. The context is D105564 which I started working on again recently. I was having difficulties finding a solution that also worked for local lambdas.

Sure, sounds OK.

Some more details on the specifics in the patch description would be good - I'm still not 100% clear on the reproduction case to understand what we're working around, exactly. Do you have a godbolt reproduction/small example, by chance?

Seems good - if folks find the growth too much, or it creates weird perf issues otherwise, we can discuss that if/when it comes up.

Sounds good

sounds ok, thanks!

(ah, might not be causing existing breakage because we're building the libc++ as a module, so stdatomic before/after doesn't interfere with the module, perhaps... )

Looks about right at a glance - thanks!

In D97044#3509541, @rnk wrote:

I wanted to give some early feedback that this broke our internal build of CPython, and we put together a local workaround, so we aren't blocked. I don't fully understand what's going on or have time to follow up, but I wanted to give a heads up that this seems like it could be a disruptive change for users.

Can't say I'm following the details - but does seem like the code is correct, yeah? Though I'm open to workarounds, within reason. Happy to help throw around ideas if someone can show me a small/workable example on godbolt, for instance.

In rG0d8cb8b399adcd17e8bf17be7814d030308c8b82#1084994, @kparzysz wrote:

Here's more info:

$ c++ --version
FreeBSD clang version 13.0.0 (git@github.com:llvm/llvm-project.git llvmorg-13.0.0-0-gd7b669b3a303)
Target: x86_64-unknown-freebsd13.0
Thread model: posix
InstalledDir: /usr/bin

Most files in /usr/include/c++/v1 match those from release/13.x:libcxx/include. Maybe there is something different in __config, but I didn't look in detail.

In D123319#3486544, @dblaikie wrote:

In D123319#3474997, @dblaikie wrote:

In D123319#3473693, @shafik wrote:

In D123319#3473283, @dblaikie wrote:

('scuse the delay)

Baseline: I'm still not really sure this is the right direction. Is there a sound argument for why this change is suitable for lambdas, but not for other types? I believe all the situations that can happen with other types can happen with lambdas (& the other way around) with sufficiently interestingly crafted inputs.

I had a couple of approaches but once I saw how gcc was handling it, I just went with consistency with gcc. I might have been missing some cases but I did not have other test case that I ran into issues with.

What's the basic reproduction of the issue? Using that I can probably produce a non-lambda example that tickles the same bug & demonstrates why this should be generalized and/or fixed in lldb instead.

Ping on this ^

In rG0d8cb8b399adcd17e8bf17be7814d030308c8b82#1084182, @kparzysz wrote:

This fails to compile:

$ /usr/bin/c++ -DGTEST_HAS_RTTI=0 -D_DEBUG -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/w/bld/org/lib/DebugInfo/DWARF -I/w/src/llvm.org/llvm/lib/DebugInfo/DWARF -I/w/bld/org/include -I/w/src/llvm.org/llvm/include -I/usr/local/include -stdlib=libc++ -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wstring-conversion -Wmisleading-indentation -ffunction-sections -fdata-sections -O3 -DNDEBUG  -fno-exceptions -fno-rtti -UNDEBUG -std=c++14 -MD -c /w/src/llvm.org/llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
In file included from /w/src/llvm.org/llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp:8:
In file included from /w/src/llvm.org/llvm/include/llvm/DebugInfo/DWARF/DWARFVerifier.h:12:
In file included from /w/src/llvm.org/llvm/include/llvm/ADT/Optional.h:19:
In file included from /w/src/llvm.org/llvm/include/llvm/ADT/Hashing.h:51:
In file included from /usr/include/c++/v1/algorithm:653:
In file included from /usr/include/c++/v1/functional:500:
In file included from /usr/include/c++/v1/__functional/function.h:20:
In file included from /usr/include/c++/v1/__memory/shared_ptr.h:22:
/usr/include/c++/v1/__memory/allocator.h:154:28: error: call to implicitly-deleted copy constructor of 'llvm::IntervalMap<unsigned int, unsigned long, 12>'
        ::new ((void*)__p) _Up(_VSTD::forward<_Args>(__args)...);
                           ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/c++/v1/__memory/allocator_traits.h:290:13: note: in instantiation of function template specialization 'std::allocator<llvm::IntervalMap<unsigned int, unsigned long, 12>>::construct<llvm::IntervalMap<unsigned int, unsigned long, 12>, const llvm::IntervalMap<unsigned int, unsigned long, 12> &>' requested here
        __a.construct(__p, _VSTD::forward<_Args>(__args)...);
            ^
/usr/include/c++/v1/vector:1085:25: note: in instantiation of function template specialization 'std::allocator_traits<std::allocator<llvm::IntervalMap<unsigned int, unsigned long, 12>>>::construct<llvm::IntervalMap<unsigned int, unsigned long, 12>, const llvm::IntervalMap<unsigned int, unsigned long, 12> &, void>' requested here
        __alloc_traits::construct(this->__alloc(), _VSTD::__to_address(__pos), __x);
                        ^
/usr/include/c++/v1/vector:1178:9: note: in instantiation of member function 'std::vector<llvm::IntervalMap<unsigned int, unsigned long, 12>>::__construct_at_end' requested here
        __construct_at_end(__n, __x);
        ^
/w/src/llvm.org/llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp:410:48: note: in instantiation of member function 'std::vector<llvm::IntervalMap<unsigned int, unsigned long, 12>>::vector' requested here
  std::vector<IntervalMap<uint32_t, uint64_t>> Sections(
                                               ^
/w/src/llvm.org/llvm/include/llvm/ADT/IntervalMap.h:970:14: note: copy constructor of 'IntervalMap<unsigned int, unsigned long, 12>' is implicitly deleted because variant field 'leaf' has a non-trivial copy constructor
    RootLeaf leaf;
             ^
1 error generated.

In D123813#3494994, @llvm-commits wrote:

(as you already noticed this was approved).

I bet this was manually marked because somehow our phabricator instance sometimes takes > 1 hour to recognize something as being committed and automatically add it. I also ended up manually adding a revision a couple of times thinking the automation was broken until I realized that it's just really slow.

Yeah, mutating the DICompileUnit at this point seems a bad idea - it could end up inconsistent between things that happened before the mutation V after, etc.

Sure, if that works.

Doesn't look like all the cases are tested? (eg: I searched for basic_iostream and I only see the source change but no test change) - I realize some of these may be previously missing test coverage, but would you mind checking/adding the missing coverage?

In D124892#3494463, @ayermolo wrote:

@dblaikie Do you think you can review this, or someone else that you think would be better?

In D112175#3489258, @dexonsmith wrote:

In D112175#3489247, @dexonsmith wrote:

Thanks for running that. I just pushed https://github.com/dexonsmith/llvm-project/tree/perf/small-vector-specialize-reserveForParamAndGetAddressImpl, which should cause compile-time results to show up at http://llvm-compile-time-tracker.com/index.php?config=NewPM-O3&stat=instructions&remote=dexonsmith eventually, and we can see if there's a meaningful impact outside of the benchmark. Not sure whether it's worth changing push_back() since even on the toy benchmark it's not a big difference.

Also, it'd be interesting to look at what is different about the CodeGen, and hunt down why the optimizations are missed. Seems better to fix the optimizer than to add redundant specializations all over the place.

Tried this patch on some internal targets and metrics and it seems actually quite reasonable.

In D123319#3474997, @dblaikie wrote:

In D123319#3473693, @shafik wrote:

In D123319#3473283, @dblaikie wrote:

('scuse the delay)

Baseline: I'm still not really sure this is the right direction. Is there a sound argument for why this change is suitable for lambdas, but not for other types? I believe all the situations that can happen with other types can happen with lambdas (& the other way around) with sufficiently interestingly crafted inputs.

I had a couple of approaches but once I saw how gcc was handling it, I just went with consistency with gcc. I might have been missing some cases but I did not have other test case that I ran into issues with.

What's the basic reproduction of the issue? Using that I can probably produce a non-lambda example that tickles the same bug & demonstrates why this should be generalized and/or fixed in lldb instead.

In rG75881d8b023ef8576afbba510c07eb8b899f2ec7#1082049, @aeubanks wrote:

many returned analyses can't be immutable, passes can either choose to invalidate them or manually keep them up to date, making them const wouldn't allow us to keep them up to date manually

Yeah, I'd go with the fix at memcpy in D122604 instead.

Would an explicit naming be more suitable than a region start/end? (I'd have considered this feedback for D116503 too, but didn't catch that one in review) The region based thing makes non-positional arguments weirdly positional (not that these are the first instances of that in compiler/linker tools, I don't think) - and I guess the override behavior is already positional to some degree.

In D124370#3474824, @labath wrote:

In D124370#3472394, @clayborg wrote:

So I believe the reason we need to be able to add methods to a class is for templates. Templates in DWARF are really poorly emitted all in the name of saving space. There is no full definition of template classes that get emitted, just a class definition with _only_ the methods that the user used in the current compile unit. DWARF doesn't really support emitting a templatized definition of a class in terms of a type T, it will always emit instantiations of the type T directly. So in LLDB we must create a template class like "std::vector<int>" and say it has no member functions, and then add each member function as a type specific specialization due to how the types must be created in the clang::ASTContext.

in one DWARF compile unit you end up having say "std::vector<int>::clear()" and in another you would have "std::vector<int>::erase()". In order to make the most complete definition of template types we need to find all "std::vector<int>" definitions and manually add any method that we haven't already seen, otherwise we end up not being able to call methods that might exist. Of course calling template functions is already fraught with danger since most are inlined if they come from the STL, but if the user asks for the class definition for "std::vector<int>", we should show all that we can. Can you make sure this patch doesn't hurt that functionality? We must have a test for it.

So we can't just say we will accept just one class definition if we have template types. Given this information, let me know what you think of your current patch

I think I'm confused. I know we're doing some funny stuff with class templates, and I'm certain I don't fully understand how it works. However, I am also pretty sure your description is not correct either. A simple snippet like std::vector<int> v; will produce a definition for the vector<int> class, and that definition will include the declarations for erase, clear, and any other non-template member function, even though the code definitely does not call them. And this makes perfect sense to me given how DWARF (and clang) describes only template instantiations -- so the template instantiation vector<int> is treated the same way as a class vector_int with the same interface.

What you're describing resembles the treatment of template member functions -- those are only emitted in the compile units that they are used. This is very unfortunate for us, though I think it still makes sense from the DWARF perspective. However:
a) In line with the previous paragraph -- this behavior is the same for template class instantiations and regular classes. IOW, the important question is whether the method is a template, not whether its enclosing class is
b) (precisely for this reason) we currently completely ignore member function templates when parsing classes
Given all of that, I don't see how templates are relevant for this patch. *However*, please read on.