Page MenuHomePhabricator

dblaikie (David Blaikie)
User

Projects

User Details

User Since
Oct 8 2012, 9:19 AM (502 w, 4 d)

Recent Activity

Today

dblaikie added a comment to D113741: [RFC][DwarfDebug][AsmPrinter] Support emitting function-local declaration for a lexical block.

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.

Fri, May 27, 1:49 PM · Restricted Project, debug-info, Restricted Project
dblaikie added inline comments to D125695: [clang][DebugInfo] Allow function-local type-like entities to be scoped within a lexical block (5/5).
Fri, May 27, 1:27 PM · debug-info, Restricted Project, Restricted Project
dblaikie added inline comments to D119051: Extend the C++03 definition of POD to include defaulted functions.
Fri, May 27, 9:03 AM · Restricted Project, Restricted Project

Yesterday

dblaikie added inline comments to D126257: Round up zero-sized symbols to 1 byte in `.debug_aranges`..
Thu, May 26, 1:20 PM · Restricted Project, Restricted Project
dblaikie added inline comments to D119051: Extend the C++03 definition of POD to include defaulted functions.
Thu, May 26, 12:01 PM · Restricted Project, Restricted Project
dblaikie added a comment to D126257: Round up zero-sized symbols to 1 byte in `.debug_aranges`..

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).

Thu, May 26, 11:47 AM · Restricted Project, Restricted Project
dblaikie added inline comments to D126401: [ADT] Explicitly delete copy/move constructors and operator= in IntervalMap.
Thu, May 26, 11:43 AM · Restricted Project, Restricted Project

Wed, May 25

dblaikie added a comment to D124892: [DWARF] Fix split dwarf debug inlining in mix mode situation..

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.

Wed, May 25, 3:57 PM · Restricted Project, Restricted Project
dblaikie added a comment to D113741: [RFC][DwarfDebug][AsmPrinter] Support emitting function-local declaration for a lexical block.

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.

Wed, May 25, 3:29 PM · Restricted Project, debug-info, Restricted Project
dblaikie abandoned D100353: Support optnone in SCCP.

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.

Wed, May 25, 2:50 PM · Restricted Project, Restricted Project
dblaikie added a comment to D102122: Support warn_unused_result on typedefs.

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"?

Wed, May 25, 2:29 PM · Restricted Project, Restricted Project
dblaikie updated the diff for D102122: Support warn_unused_result on typedefs.

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

Wed, May 25, 2:25 PM · Restricted Project, Restricted Project
dblaikie added a comment to D113741: [RFC][DwarfDebug][AsmPrinter] Support emitting function-local declaration for a lexical block.

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

Wed, May 25, 1:55 PM · Restricted Project, debug-info, Restricted Project
dblaikie added a comment to D113741: [RFC][DwarfDebug][AsmPrinter] Support emitting function-local declaration for a lexical block.

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

Wed, May 25, 1:35 PM · Restricted Project, debug-info, Restricted Project
dblaikie added a comment to D122974: prevent ConstString from calling djbHash() more than once.

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).

Wed, May 25, 1:30 PM · Restricted Project, Restricted Project, Restricted Project
dblaikie added a comment to D126257: Round up zero-sized symbols to 1 byte in `.debug_aranges`..

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.

Wed, May 25, 1:12 PM · Restricted Project, Restricted Project
dblaikie updated the diff for D102122: Support warn_unused_result on typedefs.

Rebase

Wed, May 25, 12:46 PM · Restricted Project, Restricted Project
dblaikie accepted D126401: [ADT] Explicitly delete copy/move constructors and operator= in IntervalMap.

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.

Wed, May 25, 12:38 PM · Restricted Project, Restricted Project
dblaikie added inline comments to D119051: Extend the C++03 definition of POD to include defaulted functions.
Wed, May 25, 11:19 AM · Restricted Project, Restricted Project
dblaikie accepted D125611: DWARFVerifier: Change vector of IntervalMap to vector of unique_ptrs.

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

Wed, May 25, 10:03 AM · Restricted Project, Restricted Project
dblaikie added inline comments to D126159: [ADT] Add edit_distance_insensitive to StringRef.
Wed, May 25, 10:03 AM · Restricted Project, Restricted Project

Tue, May 24

dblaikie added a comment to D113741: [RFC][DwarfDebug][AsmPrinter] Support emitting function-local declaration for a lexical block.

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)

Tue, May 24, 8:38 PM · Restricted Project, debug-info, Restricted Project
dblaikie added inline comments to D117616: GCC ABI Compatibility: Preserve alignment of non-pod members in packed structs.
Tue, May 24, 8:37 PM · Restricted Project, Restricted Project
dblaikie added a comment to D126257: Round up zero-sized symbols to 1 byte in `.debug_aranges`..

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).

Tue, May 24, 8:36 PM · Restricted Project, Restricted Project
Herald added a project to D102122: Support warn_unused_result on typedefs: Restricted Project.

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?

Tue, May 24, 8:35 PM · Restricted Project, Restricted Project
dblaikie added a comment to D119051: Extend the C++03 definition of POD to include defaulted functions.

Ping

Tue, May 24, 8:13 PM · Restricted Project, Restricted Project
dblaikie updated the diff for D119051: Extend the C++03 definition of POD to include defaulted functions.

Rebase

Tue, May 24, 8:13 PM · Restricted Project, Restricted Project
dblaikie updated the diff for D118511: Add a warning for not packing non-POD members in packed structs.

rebase

Tue, May 24, 8:08 PM · Restricted Project, Restricted Project
dblaikie added a comment to D118511: Add a warning for not packing non-POD members in packed structs.

@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?

Tue, May 24, 8:08 PM · Restricted Project, Restricted Project
dblaikie committed rGe59f648d698e: Move GCC-compatible pod-packing change to v15/old behavior available at v14 and… (authored by dblaikie).
Move GCC-compatible pod-packing change to v15/old behavior available at v14 and…
Tue, May 24, 8:04 PM · Restricted Project, Restricted Project
dblaikie closed D126334: Move GCC-compatible pod-packing change to v15/old behavior available at v14 and below.
Tue, May 24, 8:03 PM · Restricted Project, Restricted Project
dblaikie accepted D126257: Round up zero-sized symbols to 1 byte in `.debug_aranges`..

How's this?

Tue, May 24, 5:03 PM · Restricted Project, Restricted Project
dblaikie added a comment to D122974: prevent ConstString from calling djbHash() more than once.

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

Tue, May 24, 3:39 PM · Restricted Project, Restricted Project, Restricted Project
dblaikie added a comment to D126309: [docs][OpaquePtr] Add detail to motivations behind opaque pointers.

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

Tue, May 24, 3:31 PM · Restricted Project, Restricted Project
dblaikie added inline comments to D117616: GCC ABI Compatibility: Preserve alignment of non-pod members in packed structs.
Tue, May 24, 3:16 PM · Restricted Project, Restricted Project
dblaikie requested review of D126334: Move GCC-compatible pod-packing change to v15/old behavior available at v14 and below.
Tue, May 24, 3:16 PM · Restricted Project, Restricted Project
dblaikie added a comment to D125899: [ADT] Add copy constructor to IntervalMap.

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.

Tue, May 24, 2:49 PM · Restricted Project, Restricted Project
dblaikie added a comment to D125611: DWARFVerifier: Change vector of IntervalMap to vector of unique_ptrs.

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).

Tue, May 24, 2:48 PM · Restricted Project, Restricted Project
dblaikie added inline comments to D126159: [ADT] Add edit_distance_insensitive to StringRef.
Tue, May 24, 1:14 PM · Restricted Project, Restricted Project

Mon, May 23

dblaikie committed rG6c12ae8163c7: Exposes interface to free up caching data structure in DWARFDebugLine and… (authored by netforce).
Exposes interface to free up caching data structure in DWARFDebugLine and…
Mon, May 23, 8:23 PM · Restricted Project, Restricted Project
dblaikie closed D90006: Exposes interface to free up caching data structure in DWARFDebugLine and DWARFUnit for memory management.
Mon, May 23, 8:23 PM · Restricted Project, debug-info, Restricted Project
dblaikie added a comment to D126010: Make sure the AsmPrinter doesn't emit any zero-sized symbols to `.debug_aranges`..

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

Mon, May 23, 5:34 PM · Restricted Project, Restricted Project
dblaikie added inline comments to D126257: Round up zero-sized symbols to 1 byte in `.debug_aranges`..
Mon, May 23, 5:32 PM · Restricted Project, Restricted Project
dblaikie added inline comments to D126257: Round up zero-sized symbols to 1 byte in `.debug_aranges`..
Mon, May 23, 5:23 PM · Restricted Project, Restricted Project
dblaikie added a comment to D113741: [RFC][DwarfDebug][AsmPrinter] Support emitting function-local declaration for a lexical block.

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

Mon, May 23, 4:40 PM · Restricted Project, debug-info, Restricted Project
dblaikie added a comment to D126059: [Debuginfo][DWARF][NFC] Add paired methods working with DWARFDebugInfoEntry..

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.

Mon, May 23, 4:37 PM · Restricted Project, Restricted Project
dblaikie added a comment to D126159: [ADT] Add edit_distance_insensitive to StringRef.

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?

Mon, May 23, 4:30 PM · Restricted Project, Restricted Project
dblaikie added a comment to D122974: prevent ConstString from calling djbHash() more than once.

...

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?

Mon, May 23, 4:29 PM · Restricted Project, Restricted Project, Restricted Project
dblaikie added a comment to D123319: Change how we handle auto return types for lambda operator() to be consistent with gcc.

Ping

Mon, May 23, 4:23 PM · Restricted Project, Restricted Project
dblaikie added a comment to D126224: Add DWARF string debug to clang release notes..

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.

Mon, May 23, 4:05 PM · Restricted Project, Restricted Project
dblaikie added inline comments to D126010: Make sure the AsmPrinter doesn't emit any zero-sized symbols to `.debug_aranges`..
Mon, May 23, 4:03 PM · Restricted Project, Restricted Project
dblaikie added a comment to D125979: [ADT] add LockFreeDataPool class..

(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.
Mon, May 23, 3:47 PM · Restricted Project, Restricted Project

Sat, May 21

dblaikie added inline comments to D117616: GCC ABI Compatibility: Preserve alignment of non-pod members in packed structs.
Sat, May 21, 6:17 PM · Restricted Project, Restricted Project
dblaikie committed rG0b903ef6aa09: Re-add release notes for GCC ABI compatibility for non-POD in packed structs (authored by dblaikie).
Re-add release notes for GCC ABI compatibility for non-POD in packed structs
Sat, May 21, 6:16 PM · Restricted Project, Restricted Project
dblaikie added a comment to D124892: [DWARF] Fix split dwarf debug inlining in mix mode situation..

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.

Sat, May 21, 6:10 PM · Restricted Project, Restricted Project
dblaikie added a comment to D126059: [Debuginfo][DWARF][NFC] Add paired methods working with DWARFDebugInfoEntry..

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.

Sat, May 21, 5:49 PM · Restricted Project, Restricted Project
dblaikie accepted D123538: [symbolizer] Parse DW_TAG_variable DIs to show line info for globals.

Seems OK

Sat, May 21, 5:48 PM · Restricted Project, Restricted Project, Restricted Project
dblaikie added a comment to D123534: [dwarf] Emit a DIGlobalVariable for constant strings..

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

Sat, May 21, 5:41 PM · Restricted Project, Restricted Project, Restricted Project
dblaikie added a comment to D125837: [ADT] Make ArrayRef constexpr friendly.

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

Sat, May 21, 5:38 PM · Restricted Project, Restricted Project
dblaikie added a comment to D125979: [ADT] add LockFreeDataPool class..

(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?)

Sat, May 21, 5:29 PM · Restricted Project, Restricted Project
dblaikie added a comment to D126010: Make sure the AsmPrinter doesn't emit any zero-sized symbols to `.debug_aranges`..

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.

Sat, May 21, 5:28 PM · Restricted Project, Restricted Project

Thu, May 19

dblaikie added a comment to D126010: Make sure the AsmPrinter doesn't emit any zero-sized symbols to `.debug_aranges`..

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)

Thu, May 19, 3:50 PM · Restricted Project, Restricted Project

Wed, May 18

dblaikie committed rG42dac47e8708: Fix some -Wstrict-prototypes issues in ORC examples (authored by dblaikie).
Fix some -Wstrict-prototypes issues in ORC examples
Wed, May 18, 12:44 PM · Restricted Project, Restricted Project
dblaikie added a comment to D125899: [ADT] Add copy constructor to IntervalMap.

Changed the copy to a deep one.

Wed, May 18, 10:18 AM · Restricted Project, Restricted Project

Tue, May 17

dblaikie accepted D90006: Exposes interface to free up caching data structure in DWARFDebugLine and DWARFUnit for memory management.

Looks OK otherwise

Tue, May 17, 3:22 PM · Restricted Project, debug-info, Restricted Project
dblaikie accepted D125830: [Analysis] Avoid virtual dtor. NFC..

Awesome, thanks!

Tue, May 17, 1:34 PM · Restricted Project, Restricted Project
dblaikie added a comment to D125611: DWARFVerifier: Change vector of IntervalMap to vector of unique_ptrs.

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).

Tue, May 17, 1:32 PM · Restricted Project, Restricted Project

Mon, May 16

dblaikie added a comment to D123319: Change how we handle auto return types for lambda operator() to be consistent with gcc.

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.

Mon, May 16, 9:57 PM · Restricted Project, Restricted Project
dblaikie accepted D125322: [llvm][json] Fix UINT64 json parsing.

Sure, sounds OK.

Mon, May 16, 4:24 PM · Restricted Project, Restricted Project
dblaikie added a comment to D125611: DWARFVerifier: Change vector of IntervalMap to vector of unique_ptrs.

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?

Mon, May 16, 4:15 PM · Restricted Project, Restricted Project
dblaikie added inline comments to D123538: [symbolizer] Parse DW_TAG_variable DIs to show line info for globals.
Mon, May 16, 4:14 PM · Restricted Project, Restricted Project, Restricted Project
dblaikie accepted D123534: [dwarf] Emit a DIGlobalVariable for constant strings..

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.

Mon, May 16, 4:02 PM · Restricted Project, Restricted Project, Restricted Project

Fri, May 13

dblaikie accepted D125531: [DebugInfo][Test] Simplify 'llvm/test/CodeGen/ARM/*-MergedGlobalDbg.ll'. NFC.

Sounds good

Fri, May 13, 6:07 PM · Restricted Project, Restricted Project
dblaikie accepted D125535: [clang][NFC] Cleanup some coroutine tests.

sounds ok, thanks!

Fri, May 13, 6:06 PM · Restricted Project, Restricted Project

Thu, May 12

dblaikie added a comment to D97044: [libc++] [C++2b] [P0943] Add stdatomic.h header..

(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... )

Thu, May 12, 3:13 PM · Restricted Project, Restricted Project
dblaikie accepted D125257: [demangler] Avoid special-subst code duplication.

Looks about right at a glance - thanks!

Thu, May 12, 3:05 PM · Restricted Project, Restricted Project, Restricted Project
dblaikie added a comment to D97044: [libc++] [C++2b] [P0943] Add stdatomic.h header..
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.

Thu, May 12, 1:51 PM · Restricted Project, Restricted Project
dblaikie added a comment to rG0d8cb8b399ad: DWARFVerifier: Verify CU/TU index overlap issues.

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.

Thu, May 12, 10:44 AM · Restricted Project, Restricted Project

Wed, May 11

dblaikie added inline comments to D123538: [symbolizer] Parse DW_TAG_variable DIs to show line info for globals.
Wed, May 11, 1:29 PM · Restricted Project, Restricted Project, Restricted Project

Tue, May 10

dblaikie added a comment to rG0d8cb8b399ad: DWARFVerifier: Verify CU/TU index overlap issues.

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.

Tue, May 10, 10:56 PM · Restricted Project, Restricted Project

Mon, May 9

dblaikie added a project to D122270: Support converting pointers from opaque to typed: Restricted Project.
Mon, May 9, 10:44 AM · Restricted Project, Restricted Project, Restricted Project
dblaikie added a comment to D123319: Change how we handle auto return types for lambda operator() to be consistent with gcc.

('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 ^

Mon, May 9, 9:20 AM · Restricted Project, Restricted Project
dblaikie added a comment to rG0d8cb8b399ad: DWARFVerifier: Verify CU/TU index overlap issues.

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.
Mon, May 9, 9:19 AM · Restricted Project, Restricted Project

Thu, May 5

dblaikie added a comment to D123813: llvm-reduce: Clone some of the easy function properties.

(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.

Thu, May 5, 11:24 PM · Restricted Project, Restricted Project
dblaikie added a comment to D124892: [DWARF] Fix split dwarf debug inlining in mix mode situation..

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.

Thu, May 5, 1:03 PM · Restricted Project, Restricted Project
dblaikie accepted D123201: [demangler] Buffer peeking needs buffer.

Sure, if that works.

Thu, May 5, 12:48 PM · Restricted Project, Restricted Project, Restricted Project
dblaikie added a comment to D123813: llvm-reduce: Clone some of the easy function properties.
Thu, May 5, 12:24 PM · Restricted Project, Restricted Project
dblaikie updated subscribers of D123134: [demangler] No need to space adjacent template closings.

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?

Thu, May 5, 11:31 AM · Restricted Project, Restricted Project, Restricted Project
dblaikie added a comment to D124892: [DWARF] Fix split dwarf debug inlining in mix mode situation..

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

Thu, May 5, 11:29 AM · Restricted Project, Restricted Project
dblaikie committed rG0d8cb8b399ad: DWARFVerifier: Verify CU/TU index overlap issues (authored by dblaikie).
DWARFVerifier: Verify CU/TU index overlap issues
Thu, May 5, 11:19 AM · Restricted Project, Restricted Project
dblaikie added a comment to D112175: [NFC] Add llvm::StaticVector ADT.

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.

Thu, May 5, 10:37 AM · Restricted Project, Restricted Project

Tue, May 3

dblaikie updated subscribers of D123534: [dwarf] Emit a DIGlobalVariable for constant strings..

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

Tue, May 3, 12:49 PM · Restricted Project, Restricted Project, Restricted Project

Mon, May 2

dblaikie added a comment to D123319: Change how we handle auto return types for lambda operator() to be consistent with gcc.

('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.

Mon, May 2, 12:45 PM · Restricted Project, Restricted Project
dblaikie added a comment to rG75881d8b023e: [NFC] const-ed the return type of FunctionPropertiesAnalysis.

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

Mon, May 2, 12:41 PM · Restricted Project, Restricted Project
dblaikie added inline comments to D122604: [demangler] Simplify OutputBuffer initialization.
Mon, May 2, 11:13 AM · Restricted Project, Restricted Project, Restricted Project
dblaikie added a comment to D124524: [demangler] Avoid nullptr UB.

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

Mon, May 2, 11:09 AM · Restricted Project, Restricted Project
dblaikie added a comment to D121175: [clang] Add -Wstart-no-unknown-warning-option/-Wend-no-unknown-warning-option..

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.

Mon, May 2, 8:57 AM · Restricted Project, Restricted Project
Herald added a project to D116503: [clang] Add --start-no-unused-arguments/--end-no-unused-arguments to silence some unused argument warnings: Restricted Project.
Mon, May 2, 8:54 AM · Restricted Project, Restricted Project

Thu, Apr 28

dblaikie added inline comments to D122988: [BOLT][DWARF] Add version 5 split dwarf support.
Thu, Apr 28, 4:30 PM · Restricted Project, Restricted Project, Restricted Project
dblaikie added a comment to D124370: [lldb/DWARF] Fix linking direction in CopyUniqueClassMethodTypes.

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.

Thu, Apr 28, 4:13 PM · Restricted Project, Restricted Project