Page MenuHomePhabricator

dblaikie (David Blaikie)
User

Projects

User Details

User Since
Oct 8 2012, 9:19 AM (424 w, 15 h)

Recent Activity

Yesterday

dblaikie added inline comments to D88988: [llvm-symbolizer] Add inline stack traces for Windows..
Mon, Nov 23, 8:52 PM · Restricted Project, Restricted Project
dblaikie added a comment to rG2482648a795a: thinlto_embed_bitcode.ll: clarify grep should treat input as text.

Should this be using FileCheck (check for the thing you want, and check-not that it doesn't appear otherwise) instead of grep, perhaps?

Mon, Nov 23, 12:17 PM

Fri, Nov 20

dblaikie accepted D91058: [DebugInfo] Refactor code for emitting DWARF expressions for FP constants.
Fri, Nov 20, 8:02 PM · Restricted Project
dblaikie added a comment to D91900: OpaquePtr: Make byval/sret types mandatory.
Fri, Nov 20, 7:59 PM · Restricted Project
dblaikie accepted D91900: OpaquePtr: Make byval/sret types mandatory.

Generally looks good to me, thanks!

Fri, Nov 20, 6:07 PM · Restricted Project
dblaikie added a comment to D91900: OpaquePtr: Make byval/sret types mandatory.

Test coverage? (I guess all tests are already updated, but having a test showing what happens/testing the errors if these things are missing would probably be good)

Fri, Nov 20, 5:32 PM · Restricted Project
dblaikie added inline comments to D91803: [lld] Use -1 as tombstone value for discarded code ranges.
Fri, Nov 20, 1:14 PM · Restricted Project

Thu, Nov 19

dblaikie added a comment to D91781: [VE] Add regression test for D91151.

with assertions enabled/if the old code asserting, you may be able to simplify the test case down much further, as the assertion will ensure a more reliable/fast failure rather than some unspecified failure due to undefined behavior in the execution later on/elsewhere.

That's make sens. However, cast mechanism is compilcated and I couldn't find any good way to simplify this test case. Anyway, this is an assertion message and some back trace from old code.

llc: /home/jam/llvm-upstream/llvm-project/llvm/include/llvm/Support/Casting.h:262: typename llvm::cast_retty<To, From>::ret_type llvm::cast(Y&) [with X = llvm::ConstantSDNode; Y = llvm::SDValue; typename llvm::cast_retty<To, From>::ret_type = llvm::ConstantSDNode*]: Assertion `isa<X>(Val) && "cast<Ty>() argument of incompatible type!"' failed.
...
 #6 0x00007fbf5a26b859 abort (/lib/x86_64-linux-gnu/libc.so.6+0x25859)
 #7 0x00007fbf5a26b729 (/lib/x86_64-linux-gnu/libc.so.6+0x25729)
 #8 0x00007fbf5a27cf36 (/lib/x86_64-linux-gnu/libc.so.6+0x36f36)
 #9 0x0000559f57608668 llvm::cast_retty<llvm::ConstantSDNode, llvm::SDValue>::ret_type llvm::cast<llvm::ConstantSDNode, llvm::SDValue>(llvm::SDValue&) /home/jam/llvm-upstream/llvm-project/llvm/include/llvm/Support/Casting.h:262:3
#10 0x0000559f5792a5af (anonymous namespace)::VEDAGToDAGISel::selectADDRzii(llvm::SDValue, llvm::SDValue&, llvm::SDValue&, llvm::SDValue&) /home/jam/llvm-upstream/llvm-project/llvm/lib/Target/VE/VEISelDAGToDAG.cpp:230:38
#11 0x0000559f5792965d (anonymous namespace)::VEDAGToDAGISel::CheckComplexPattern(llvm::SDNode*, llvm::SDNode*, llvm::SDValue, unsigned int, llvm::SmallVectorImpl<std::pair<llvm::SDValue, llvm::SDNode*> >&) /home/jam/llvm-upstream/build-debug/lib/Target/VE/VEGenDAGISel.inc:11266:100
Thu, Nov 19, 6:55 PM · Restricted Project, Restricted Project
dblaikie added inline comments to D90507: [Driver] Add DWARF64 flag: -gdwarf64.
Thu, Nov 19, 6:38 PM · Restricted Project
dblaikie added inline comments to D90507: [Driver] Add DWARF64 flag: -gdwarf64.
Thu, Nov 19, 4:22 PM · Restricted Project
dblaikie added inline comments to D90507: [Driver] Add DWARF64 flag: -gdwarf64.
Thu, Nov 19, 1:15 PM · Restricted Project
dblaikie added a comment to D91781: [VE] Add regression test for D91151.

Hi, @dblaikie. I generated a test case which causes a segmentation fault if we didn't apply D91151 (https://reviews.llvm.org/D91151) following your suggestion. I appreciate if you have more suggestions.

Were you testing with assertions enabled? I'd expect a test case to cause an assertion failure (when the "cast" was applied to an object that wasn't the intended type) rather than a segmentation fault.

There is no assertions.

Thu, Nov 19, 1:15 PM · Restricted Project, Restricted Project
dblaikie added a comment to D91781: [VE] Add regression test for D91151.

Hi, @dblaikie. I generated a test case which causes a segmentation fault if we didn't apply D91151 (https://reviews.llvm.org/D91151) following your suggestion. I appreciate if you have more suggestions.

Thu, Nov 19, 10:58 AM · Restricted Project, Restricted Project
dblaikie added inline comments to D90507: [Driver] Add DWARF64 flag: -gdwarf64.
Thu, Nov 19, 10:49 AM · Restricted Project
dblaikie added a comment to D91803: [lld] Use -1 as tombstone value for discarded code ranges.

Are there no tests that fail under this change? If not, I guess we should add one.

Failed Tests (3):
  lld :: wasm/debug-removed-fn.ll
  lld :: wasm/debuginfo.test
  lld :: wasm/undefined-weak-call.ll

The change does not seem right. For non-debug sections (when --allow-undefined is set), addend; .debug_ranges/.debug_loc => -2; other .debug_* => -1

Thu, Nov 19, 10:21 AM · Restricted Project

Wed, Nov 18

dblaikie added a comment to D91678: Symbolizer test for Basic block sections.

Is it possible for the test case to be written in assembly and assembled during the test execution? (I realize a lot of the symbolizer tests are not written this way, but I think it's the direction we're generally moving in with these sort of tests) - good to still include the C source too, as you have.

Thinking about this a bit more,two problems to solve if I need to get this in as assembly:

  1. Ideally I want to link this with lld to use the symbol-ordering-file option to get the sections dis-contiguous.
  2. Since I use hard-coded addresses to symbolize and check, any changes to the linker that affects the output address used might break this test.

For 1) I could reorder the sections manually in the assembly to be discontiguous. I don't have a good solution for 2) other than maybe using nm to first get the address of the symbol and then symbolize it? Thoughts?

Wed, Nov 18, 9:16 PM · Restricted Project
dblaikie added inline comments to D90507: [Driver] Add DWARF64 flag: -gdwarf64.
Wed, Nov 18, 4:51 PM · Restricted Project
dblaikie added a comment to D91678: Symbolizer test for Basic block sections.

Is it possible for the test case to be written in assembly and assembled during the test execution? (I realize a lot of the symbolizer tests are not written this way, but I think it's the direction we're generally moving in with these sort of tests) - good to still include the C source too, as you have.

Wed, Nov 18, 4:42 PM · Restricted Project
dblaikie added a comment to D91722: [DebugInfo] Use variadic debug values to salvage BinOps and GEP instrs with non-const operands.

(minor drive-by comment, but I'm not the right person to do more in depth review of this patch, unfortunately)

Wed, Nov 18, 4:40 PM · debug-info, Restricted Project
dblaikie committed rG5747380772e0: Added GDB pretty printer for StringMap (authored by MoritzS).
Added GDB pretty printer for StringMap
Wed, Nov 18, 4:33 PM
dblaikie closed D91183: Added GDB pretty printer for StringMap.
Wed, Nov 18, 4:33 PM · Restricted Project
dblaikie added inline comments to D90507: [Driver] Add DWARF64 flag: -gdwarf64.
Wed, Nov 18, 2:30 PM · Restricted Project

Tue, Nov 17

dblaikie accepted D91674: ADT: Share an implementation for single-element insert in SmallVector, NFC.

Looks good

Tue, Nov 17, 6:53 PM · Restricted Project
dblaikie accepted D91670: Fix crash after looking up dwo_id=0 in CU index..

Looks good, thanks!

Tue, Nov 17, 6:52 PM · Restricted Project
dblaikie accepted D90916: [DebugInfo] Emit locations for large constant integers.

Looks good!

Tue, Nov 17, 6:05 PM · Restricted Project
dblaikie accepted D91058: [DebugInfo] Refactor code for emitting DWARF expressions for FP constants.

Looks good

Tue, Nov 17, 6:02 PM · Restricted Project
dblaikie added inline comments to D91670: Fix crash after looking up dwo_id=0 in CU index..
Tue, Nov 17, 5:54 PM · Restricted Project
dblaikie committed rG6bfb4120ead7: set the alignment of mlir::AttributeStorage to 64 bit explicitly to fix 32 bit… (authored by daquexian).
set the alignment of mlir::AttributeStorage to 64 bit explicitly to fix 32 bit…
Tue, Nov 17, 5:52 PM
dblaikie closed D91062: set the alignment of mlir::AttributeStorage to 64 bit explicitly to fix 32 bit platform.
Tue, Nov 17, 5:52 PM · Restricted Project
dblaikie added a comment to D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'.

Thanks for the walkthroughs/help. Also stared at the code a bit. I think I get it now. Some of the confusion also came from having both LPM and NPM versions of the always inliner in the same file, though they seem to share no code.

Tue, Nov 17, 5:29 PM · Restricted Project, Restricted Project
dblaikie added a comment to D91062: set the alignment of mlir::AttributeStorage to 64 bit explicitly to fix 32 bit platform.

@rriddle @dblaikie Is this patch ready to merge? Thanks!

Tue, Nov 17, 2:42 PM · Restricted Project
dblaikie added a comment to D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'.

Performing the mandatory inlinings first simplifies the problem the full inliner needs to solve

That confuses me a bit - is that suggesting that we don't run the AlwaysInliner when we are running the Inliner (ie: we only run the AlwaysInliner at -O0, and use the Inliner at higher optimization levels and let the Inliner do always inlining too)?
& sounds like this is suggesting that would change? That we would now perform always inlining separately from inlining? Maybe that's an orthogonal/separate change from one implementing the always inlining using the Inliner being run in a separate mode?

In the NPM, we didn't run the AlwaysInliner until D86988. See also the discussion there. The normal inliner pass was, and still is, taking care of the mandatory inlinings if it finds them. Of course, if we completely upfronted those (which this patch can do), then the normal inliner wouldn't need to. I'm not suggesting changing that - meaning, it's straightforward for the normal inliner to take care of mandatory and policy-driven inlinings. The idea, though, is that if we upfront the mandatory inlinings, the shape of the call graph the inliner operates over is simpler and the effects of inlining probably more easy to glean by the decision making policy. There are trade-offs, though - we can increase that "ease of gleaning" by performing more function simplification passes between the mandatory inlinings and the full inliner.

OK, so if I understand correctly with the old Pass Manager there were two separate passes (always inliner and inliner - they share some code though, yeah?)

AlwaysInlinerLegacyPass does, yes. The NPM variant doesn't.

Tue, Nov 17, 2:42 PM · Restricted Project, Restricted Project
dblaikie accepted D91183: Added GDB pretty printer for StringMap.
Tue, Nov 17, 2:31 PM · Restricted Project
dblaikie added a comment to D91311: Add new 'preferred_name' attribute..

How would that work for users - they would get error messages from the compiler using type names that don't exist in the source code? I'd have thought that would be quite confusing.

Yes, if a library author decides to say something like:

template<class _CharT, class _Traits>
class [[preferred_name(basic_string_view<char>, "ahaha I'm such a troll")]]
basic_string_view {
    ...
};

Then you might get compiler errors that are not super helpful. I don't think the fact that such nonsense is doable means that we shouldn't give this control to library authors.

Tue, Nov 17, 2:29 PM · Restricted Project, Restricted Project
dblaikie added a comment to D91311: Add new 'preferred_name' attribute..

[...]

Thanks for your detailed explanation. I did not understand the philosophy of the attribute, and it's now clear to me that it shouldn't be tied to the typedef, indeed.

There's another design approach we could follow, that would keep the attribute on the template but avoid the awkwardness of requiring the typedef to appear first: we could completely divorce this feature from typedefs. Instead, we could say the attribute is of the form preferred_name(type, "name"), where type is a specialization of the type to which the attribute is attached, and "name" is a name used in diagnostics when referring to that type, which is printed as if it's a member of the enclosing namespace (but there'd be no enforcement that such a member actually exists and declares the corresponding type). What do you think?

I think this is pretty interesting, actually. While it does seem a bit more loosey-goosey in terms of the type system, I can imagine useful applications of being able to use arbitrary strings, such as providing better diagnostics for specific types without having to actually have an associated typedef.

Tue, Nov 17, 1:52 PM · Restricted Project, Restricted Project

Mon, Nov 16

dblaikie added inline comments to D91183: Added GDB pretty printer for StringMap.
Mon, Nov 16, 7:38 PM · Restricted Project
dblaikie added a comment to D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'.

Performing the mandatory inlinings first simplifies the problem the full inliner needs to solve

That confuses me a bit - is that suggesting that we don't run the AlwaysInliner when we are running the Inliner (ie: we only run the AlwaysInliner at -O0, and use the Inliner at higher optimization levels and let the Inliner do always inlining too)?
& sounds like this is suggesting that would change? That we would now perform always inlining separately from inlining? Maybe that's an orthogonal/separate change from one implementing the always inlining using the Inliner being run in a separate mode?

In the NPM, we didn't run the AlwaysInliner until D86988. See also the discussion there. The normal inliner pass was, and still is, taking care of the mandatory inlinings if it finds them. Of course, if we completely upfronted those (which this patch can do), then the normal inliner wouldn't need to. I'm not suggesting changing that - meaning, it's straightforward for the normal inliner to take care of mandatory and policy-driven inlinings. The idea, though, is that if we upfront the mandatory inlinings, the shape of the call graph the inliner operates over is simpler and the effects of inlining probably more easy to glean by the decision making policy. There are trade-offs, though - we can increase that "ease of gleaning" by performing more function simplification passes between the mandatory inlinings and the full inliner.

Mon, Nov 16, 7:35 PM · Restricted Project, Restricted Project
dblaikie added inline comments to D90507: [Driver] Add DWARF64 flag: -gdwarf64.
Mon, Nov 16, 7:28 PM · Restricted Project
dblaikie added a comment to D91151: [VE][NFC] Change cast to dyn_cast.

Can you add a test case for this change?

Mon, Nov 16, 7:26 PM · Restricted Project, Restricted Project
dblaikie accepted D91505: ELFAsmParser: Remove non-SHF_ALLOC or non-executable sections' line info/address ranges contribution for -g.

Sounds good to me (I guess ALLOC and EXECINSTR can both be enabled separately? Is this consistent with the Binutils change? (or does one subsume the other?))

Mon, Nov 16, 7:07 PM · Restricted Project
dblaikie added inline comments to D91505: ELFAsmParser: Remove non-SHF_ALLOC or non-executable sections' line info/address ranges contribution for -g.
Mon, Nov 16, 4:34 PM · Restricted Project
dblaikie added a comment to D91505: ELFAsmParser: Remove non-SHF_ALLOC or non-executable sections' line info/address ranges contribution for -g.

The new behavior seems reasonable and matches GNU as 2.36 https://sourceware.org/bugzilla/show_bug.cgi?id=26850

Mon, Nov 16, 4:20 PM · Restricted Project
dblaikie added a comment to D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'.

Performing the mandatory inlinings first simplifies the problem the full inliner needs to solve

Mon, Nov 16, 4:20 PM · Restricted Project, Restricted Project

Fri, Nov 13

dblaikie updated subscribers of D90345: [DebugInfo] Fix ICE in DwarfCompileUnit::constructSubprogramScopeDIE.

@aprantl @JDevlieghere you two have more investment and familiarity with the verifier - mind checking this one over?

Fri, Nov 13, 10:25 AM · Restricted Project

Wed, Nov 11

dblaikie added a comment to D91043: [DebugInfo] Fix convert-loclist.ll.
In D91043#2390184, @rnk wrote:

This test seems fairly specific, and a bit fragile. I don't think we will get much benefit from trying to generalize it to non-x86 targets.

Wed, Nov 11, 6:04 PM · Restricted Project
dblaikie added a comment to D91203: [WebAssembly] Fixed wasm64 DWARF using 64-bit code pointer sizes.

@dblaikie Confusingly, these are all different: function pointers at runtime (in a Wasm VM) are 32-bit indices. LLVM function pointers are 64-bit in wasm64 for consistency, but get truncated when lowered in Isel. Then here we have a 3rd kind of code pointer, just for DWARF, since Wasm doesn't have the concept of a pointer to an instruction inside a function (which DWARF needs for DW_AT_low_pc, and we need to relocate).

And yes, if this is also used for globals, and there is an architecture for which code pointers would be smaller than global pointers, then this patch would break them.

An alternative solution for Wasm would be to just go along with the expected 64-bit size for DW_FORM_addr, but that would mean wasted space, and a new reloc type for us. Also means DWARF data between wasm32 and wasm64 is different.

Not sure we're perhaps talking about the same thing or there's some overlap/confusion.

What's the actual sizeof(int*) and sizeof(int(*)()) if I printed those out in some C++ code compiled to wasm? The size has to be known by the frontend/at the language level, so it can't change due to lowering.
If I have an array of int*, that array needs elements of a size known by the frontend/language-level, similarly if I have an array of int(*)() - and in both cases I'd need to use some relocation to fill in my array initializer that initializes that array?

At the C/C++/llvm layers all pointer are the same size (i.e. 64bit on wasm64), including function pointers. The difference with wasm is that those functions pointers are not in the same address space as the bytecode offsets used in the DWARF information.

Wed, Nov 11, 1:44 PM · Restricted Project
dblaikie added inline comments to D91183: Added GDB pretty printer for StringMap.
Wed, Nov 11, 12:56 PM · Restricted Project
dblaikie accepted D91062: set the alignment of mlir::AttributeStorage to 64 bit explicitly to fix 32 bit platform.
Wed, Nov 11, 12:50 PM · Restricted Project
dblaikie added a comment to D91043: [DebugInfo] Fix convert-loclist.ll.

I'm a bit confused - looks like there's a bunch of other uses of %llc_dwarf in this test directory - any idea why those ones don't fail in the same way/for you?

Wed, Nov 11, 12:49 PM · Restricted Project
dblaikie committed rG9f310bec34a2: Add missing override (& fix an else-after-return while I'm here) (authored by dblaikie).
Add missing override (& fix an else-after-return while I'm here)
Wed, Nov 11, 12:29 PM
dblaikie added a comment to D91229: [FileCheck] Flip -allow-unused-prefixes to false by default.

because FileCheck won't accept more than one occurrence of --allow-unused-prefixes

Wed, Nov 11, 9:47 AM · Restricted Project, Restricted Project

Tue, Nov 10

dblaikie added a comment to D91203: [WebAssembly] Fixed wasm64 DWARF using 64-bit code pointer sizes.

@dblaikie Confusingly, these are all different: function pointers at runtime (in a Wasm VM) are 32-bit indices. LLVM function pointers are 64-bit in wasm64 for consistency, but get truncated when lowered in Isel. Then here we have a 3rd kind of code pointer, just for DWARF, since Wasm doesn't have the concept of a pointer to an instruction inside a function (which DWARF needs for DW_AT_low_pc, and we need to relocate).

And yes, if this is also used for globals, and there is an architecture for which code pointers would be smaller than global pointers, then this patch would break them.

An alternative solution for Wasm would be to just go along with the expected 64-bit size for DW_FORM_addr, but that would mean wasted space, and a new reloc type for us. Also means DWARF data between wasm32 and wasm64 is different.

Tue, Nov 10, 9:04 PM · Restricted Project
dblaikie added a comment to D90719: [DebugInfo] Modify ctor homing as workaround for unconstructed libcxx types.

+ @ldionne for libc++ input?

To summarize, this constructor homing debug info optimization makes the assumption that if a class is being used then its constructor must have been called at some point. We noticed that some libc++ types (such as __hash_node) are nontrivial but the constructor is never called (instead there's some allocation and then the members are constructed separately).

Basically we're not sure if this can be fixed from the libc++ side but it would be nice to have some more input about this. For example would it be possible to call the constructor when __hash_node is created?

My guess would be that this doesn't come up often enough to merit an attribute, etc, and that libc++ is fixable. (if the code really wants to do no work when constructing, changing the type to have a trivial ctor and the places that want non-trivial construction can initialize the members as needed seems like it'd be viable)

I looked at the code again and __hash_node also has nontrivial members, so maybe it's not as feasible to make it a nontrivial constructor.

Tue, Nov 10, 6:04 PM · Restricted Project
dblaikie added a comment to D91203: [WebAssembly] Fixed wasm64 DWARF using 64-bit code pointer sizes.

Does this mean, for instance, that sizeof(int*) is different from sizeof(void(*)())?

Tue, Nov 10, 1:30 PM · Restricted Project

Mon, Nov 9

dblaikie committed rG724877e219cd: Roll otherwise-unused variable into assert (authored by dblaikie).
Roll otherwise-unused variable into assert
Mon, Nov 9, 11:34 PM
dblaikie accepted D90840: [lldb/DWARF] Fix sizes of DW_OP_const[1248][us] and DW_OP_litN results.

Sounds good

Mon, Nov 9, 7:11 PM · Restricted Project
dblaikie added inline comments to D91062: set the alignment of mlir::AttributeStorage to 64 bit explicitly to fix 32 bit platform.
Mon, Nov 9, 7:10 PM · Restricted Project
dblaikie added inline comments to D91062: set the alignment of mlir::AttributeStorage to 64 bit explicitly to fix 32 bit platform.
Mon, Nov 9, 4:55 PM · Restricted Project

Fri, Nov 6

dblaikie added inline comments to D90840: [lldb/DWARF] Fix sizes of DW_OP_const[1248][us] and DW_OP_litN results.
Fri, Nov 6, 5:05 PM · Restricted Project
dblaikie accepted D90989: [llvm] Check the debug info line table for basic block sections..

Looks OK - though you might want to check with -v output or otherwise check the addresses maybe? It might be the llvm-dwarfdump output, even with -v, is a bit hard to test what you may want to test on an object file - validating which line table entries are referring to which sections. If you want to test at that level to ensure the portions of the line table refer to the right bb sections, it might involve enhancing llvm-dwarfdump -v -debug-line output to include the name of the section (& section number, in cases where the name may not be unique (-fno-unique-section-names)) in a similar way to llvm-dwarfdump's output for debug_ranges/loc/etc.

Fri, Nov 6, 4:40 PM · Restricted Project
dblaikie added inline comments to D90448: [clang] Add type check for explicit instantiation of static data members.
Fri, Nov 6, 4:37 PM · Restricted Project
dblaikie added a comment to D90714: [clang]Fix length threshold for MicrosoftMangle md5 hash.

you're right about the regex syntax. i think the 4088 is over the max repetition count and there's also a problem with balanced. not sure what i did wrong here. if you have a chance to look at this otherwise i'll toil over the weekend, i'm off for the day--cheers. here's what i'm trying now just to see if it would work. can i nest repetition counts?

63:20: error: invalid regex: braces not balanced
// CHECK-DAG: @"?{{\(z\){255}}}@@3HA" = dso_local global i32 1515, align 4

i also tried it without the parens around z or with parens around z but not backslash.

Fri, Nov 6, 4:33 PM · Restricted Project
dblaikie added inline comments to D90916: [DebugInfo] Emit locations for large constant integers.
Fri, Nov 6, 11:29 AM · Restricted Project
dblaikie added a comment to D87011: [DebugInfo] Add the -dwarf64 switch to llc and other internal tools (4/19)..

At least in LLD, it's not quite as simple as being added after the user's code: if a library appears on the link line it will be included in the output order as soon as it is determined it is needed. Thus if you have have three modules 1.o, 2.o, and 3.o, with 3.o in an archive 3.a and 1.o requiring 3.o, you end up with an output order of 1.o 3.o 2.o if the input order was 1.o 3.a 2.o or 3.a 1.o 2.o or an output order of 1.o 2.o 3.o if the input order was 1.o 2.o 3.a. In fact, with use of the --undefined linker switch, you can even force 3.o to appear first.

I accept using --undefined or rearranging the command-line order is less than ideal, but I'm really not convinced LLD should have any place in parsing the DWARF to determine output order. Furthermore, it's not even a reliable solution - if the objects built with DWARF32 (potentially all of which might have come from libraries) are large enough, no amount of reordering will fix the behaviour. I think users who need DWARF64 in their libraries are just going to have to request DWARF64 versions of the libraries, if the --undefined and reordering command line options are insufficient.

I'd guess that for a large-scale project the recommendation to use -u would be unrealistic. We are talking about projects where debugging information in a single section can easily go beyond the 4GiB limit; it is impossible for the developer to adjust the command line manually.

By the way, from a semantic point of view, I don't think it matters if the DWARF is in a different order to the data it represents - I'm just concerned about the maintenance and performance burden of having to parse the DWARF to achieve this reordering.

There is no need to parse the debug info sections. Reading only the first 4 bytes of .debug_info is enough to assess the format (there might be input files with format intermixing, but we can ignore them in the sack of simplicity). And we do not need any automatic sorting if the size of an output section is less than 4GiB.

Exactly. Not to mention, I think for users that actually worry about 4Gig limit they have pretty complex build system that will need to be modified to get build order right. Probably doable, but looking at overall compilation pipeline, is it really the best approach? Within lld we don't have to parse entire debug section, just read few bytes in each CU to determine if it's 32 or 64 bit.
Yes theoretically it is possible that there are just so many third party libraries that they will over flow 4gig by themselves, but I think common case is they will be under 4 gigs.

Fri, Nov 6, 11:21 AM · debug-info, Restricted Project
dblaikie accepted D90714: [clang]Fix length threshold for MicrosoftMangle md5 hash.

Generally looks good, thanks!

Fri, Nov 6, 10:47 AM · Restricted Project

Thu, Nov 5

dblaikie added a comment to D90719: [DebugInfo] Modify ctor homing as workaround for unconstructed libcxx types.

Perhaps we could address both the UB and the debug info homing issue by adding a new attribute on the libc++ types that says it's valid to create an instance of the type without calling a constructor? (If we want to support old libc++ with trunk clang, we could detect those types and automatically synthesize the attribute, but libc++ is moving towards being version-locked to clang anyway, so I don't think we need to worry about that too much.)

Thu, Nov 5, 7:57 PM · Restricted Project
dblaikie accepted D90874: [test] Properly test -Werror-implicit-function-declaration and -Wvec-elem-size.

Looks good - thanks!

Thu, Nov 5, 7:43 PM · Restricted Project
dblaikie added inline comments to D90717: [llvm] Add a test for debug info generated with split functions..
Thu, Nov 5, 7:39 PM · Restricted Project
dblaikie added inline comments to D90874: [test] Properly test -Werror-implicit-function-declaration and -Wvec-elem-size.
Thu, Nov 5, 5:41 PM · Restricted Project
dblaikie added inline comments to D90717: [llvm] Add a test for debug info generated with split functions..
Thu, Nov 5, 5:23 PM · Restricted Project
dblaikie accepted D90894: [STLExtras] Add append_range helper..

Looks good!

Thu, Nov 5, 4:08 PM · Restricted Project
dblaikie committed rG99e64623ec9b: Do not construct std::string from nullptr (authored by georgthegreat).
Do not construct std::string from nullptr
Thu, Nov 5, 3:23 PM
dblaikie closed D87697: Do not construct std::string from nullptr.
Thu, Nov 5, 3:23 PM · Restricted Project
dblaikie added a comment to D90874: [test] Properly test -Werror-implicit-function-declaration and -Wvec-elem-size.

Thanks for taking a look at this!

Thu, Nov 5, 3:04 PM · Restricted Project
dblaikie added a comment to D90882: [SmallVector] Add range versions of append/assign..

Maybe these would be more effective as generic range-based functions? (is "append(begin, end)" part of the container concept? Not 100% clear to me on a quick check) - would make it usable with a variety of containers in one go instead of adding it to individual containers (& not being able to add it to standard containers)?

Thu, Nov 5, 2:48 PM · Restricted Project
dblaikie added a comment to D90719: [DebugInfo] Modify ctor homing as workaround for unconstructed libcxx types.
In D90719#2377324, @rnk wrote:

I had another thought, which is that even if it is UB, perhaps we really shouldn't be using UB as the basis for debug info emission. All programs have bugs, and most bugs invoke some form of UB. If we don't provide sufficient info when UB is involved, it can become harder to find the UB. The vtable type homing heuristic works because violating the heuristic assumptions typically results in a link error. Creating an object without calling the class's constructor is the kind of UB that is likely to manifest as runtime errors.

Which is to say, I'm in favor of Amy's change as written.

Thu, Nov 5, 2:31 PM · Restricted Project
dblaikie added a comment to D90714: [clang]Fix length threshold for MicrosoftMangle md5 hash.

I'm sorry, I don't see how to build up an identifier with an arbitrary number of characters using the token pasting, for example an ident with 4095 characters is not a power of 2. I tried pasting together X2048 X1024 X512 etc but that doesn't work

fu.cpp:12:18: error: pasting ")" and "X16" does not give a valid preprocessing token
 #define FUN X32(x) ## X16(x)
                  ^
fu.cpp:13:1: note: in expansion of macro ‘FUN’
Thu, Nov 5, 2:30 PM · Restricted Project
dblaikie added a comment to D87697: Do not construct std::string from nullptr.

@dblaikie, so, could you, please, commit this at last?

Thu, Nov 5, 12:22 PM · Restricted Project
dblaikie added inline comments to rGc6a384df1f8a: [Sema] Special case -Werror-implicit-function-declaration and reject other….
Thu, Nov 5, 11:54 AM
dblaikie added a comment to D90714: [clang]Fix length threshold for MicrosoftMangle md5 hash.
In D90714#2376667, @rnk wrote:

lgtm

I think the test case looks good as is, FWIW. Token pasting might make it more readable, but it's improving the existing test, and not necessarily in scope for this patch.

Thu, Nov 5, 11:40 AM · Restricted Project
dblaikie accepted D87697: Do not construct std::string from nullptr.

Good for me - please wait for @mclow.lists to weigh in/close the loop as well, though.

Thu, Nov 5, 11:32 AM · Restricted Project
dblaikie added a comment to D90714: [clang]Fix length threshold for MicrosoftMangle md5 hash.

Since the same code is used to mangle all these things, probably just test one of them?

Could use macros to stamp out longer names without having to write them out manually?

Not sure what technique to use to stamp out longer names, I tried using token pasting but that doesn't work because the tokens aren't macro expanded before pasting?
#define X50 xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
#define X45 xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
#define X100 X50 ## X50 // X100 is X50X50

Thu, Nov 5, 11:30 AM · Restricted Project
dblaikie added a comment to D90448: [clang] Add type check for explicit instantiation of static data members.

How's this compare to the similar checks for variable templates? Is there some code/checking we could share here?

Thu, Nov 5, 10:47 AM · Restricted Project
dblaikie added a comment to D90719: [DebugInfo] Modify ctor homing as workaround for unconstructed libcxx types.
In D90719#2376388, @rnk wrote:

My understanding is that such code is UB, is that right?

I guess I'm not convinced it's UB, and need some language lawyering help to decide.

Thu, Nov 5, 10:46 AM · Restricted Project

Wed, Nov 4

dblaikie accepted D90638: Introduce -dot-cfg-mssa option which creates dot-cfg style file with mssa comments included in source.

Sounds good to me then!

Wed, Nov 4, 6:44 PM · Restricted Project
dblaikie accepted D90631: [FileCheck] Use %ProtectFileCheckOutput in allow-unused-prefixes.txt.

Looks good to me!

Wed, Nov 4, 6:19 PM · Restricted Project
dblaikie added a comment to D87697: Do not construct std::string from nullptr.

I would rather see:

llvm_unreachable("unexpected type");
return ""; // or string()

i.e, keep the call to unreachable.

Wed, Nov 4, 6:18 PM · Restricted Project
dblaikie added inline comments to D90717: [llvm] Add a test for debug info generated with split functions..
Wed, Nov 4, 4:01 PM · Restricted Project
dblaikie added a comment to D90638: Introduce -dot-cfg-mssa option which creates dot-cfg style file with mssa comments included in source.

Seems pretty reasonable to me - @asbirlea could you check the test case to see the expected output, etc, makes sense? MSSA itself is not so much my wheelhouse so I'm not rightly sure myself.

Wed, Nov 4, 3:52 PM · Restricted Project
dblaikie added a comment to D90714: [clang]Fix length threshold for MicrosoftMangle md5 hash.

Since the same code is used to mangle all these things, probably just test one of them?

Wed, Nov 4, 3:51 PM · Restricted Project
dblaikie added inline comments to D90717: [llvm] Add a test for debug info generated with split functions..
Wed, Nov 4, 3:44 PM · Restricted Project

Tue, Nov 3

dblaikie added a comment to D90719: [DebugInfo] Modify ctor homing as workaround for unconstructed libcxx types.
In D90719#2372554, @rnk wrote:

Does Chromium need this fixed in clang? Or if it were fixed in libc++ would that be adequate? (does Chromium's build need to work with old libc++s, or does it always build with a libc++ that matches the compiler? (in the latter case, a fix in libc++ would be as good as a fix in clang))

Well, we'd like to make this new type homing behavior the default, and it wouldn't work with old libc++ versions, and there is the general possibility that there is code out there like this libc++ code that has implicit nontrivial constructors that are not used. So, I wouldn't think so much about what's right for Chrome, and more about what's right for Clang.

Tue, Nov 3, 5:41 PM · Restricted Project
dblaikie added a comment to D90638: Introduce -dot-cfg-mssa option which creates dot-cfg style file with mssa comments included in source.

Needs some test coverage?

Tue, Nov 3, 3:09 PM · Restricted Project
dblaikie added a comment to D90006: Exposes interface to free up caching data structure in DWARFDebugLine and DWARFUnit for memory management.

One thing to note is that a line table can be shared by multiple dwarf units. This regularly happens with type units. Theoretically, compile units can share a line table too, though that would be a pretty unusual setup...

FWIW, that does come up for LLVM when doing any kind of LTO + non-integrated assembler. (the assembly syntax doesn't provide a way to describe two distinct line tables, so both CUs end up having to refer to one line table)

Interesting. I am moderately surprised that this does not cause any problems in the consumers. I don't know if this would actually break anything, but I believe it will cause lldb to (re)parse the line table for each CU that refers to it.

Tue, Nov 3, 3:07 PM · debug-info, Restricted Project
dblaikie added a comment to D90719: [DebugInfo] Modify ctor homing as workaround for unconstructed libcxx types.

Does Chromium need this fixed in clang? Or if it were fixed in libc++ would that be adequate? (does Chromium's build need to work with old libc++s, or does it always build with a libc++ that matches the compiler? (in the latter case, a fix in libc++ would be as good as a fix in clang))

Tue, Nov 3, 3:03 PM · Restricted Project
dblaikie added a comment to D90714: [clang]Fix length threshold for MicrosoftMangle md5 hash.

Test case?

Tue, Nov 3, 3:01 PM · Restricted Project

Mon, Nov 2

dblaikie added a comment to D83655: [AsmPrinter] Split up .gcc_except_table.

(FWIW, I'd personally be in favor of -funique-section-names applying here - the specific textual description of the flag in documentation I don't think should necessarily be held as definitive, but possibly descriptive (perhaps it describes correctly the behavior as it exists/was implemented, and could be updated to include this new behavior) - seems like if someone's passing that flag it's probably because they don't want or may not have toolchain support for non-unique section names. It'd be pretty quirky/odd situation where they didn't want unique section names only for some sections and not for others.

Mon, Nov 2, 12:53 PM · Restricted Project
dblaikie added inline comments to D90631: [FileCheck] Use %ProtectFileCheckOutput in allow-unused-prefixes.txt.
Mon, Nov 2, 11:04 AM · Restricted Project
dblaikie added inline comments to D90631: [FileCheck] Use %ProtectFileCheckOutput in allow-unused-prefixes.txt.
Mon, Nov 2, 10:46 AM · Restricted Project
dblaikie added inline comments to D90631: [FileCheck] Use %ProtectFileCheckOutput in allow-unused-prefixes.txt.
Mon, Nov 2, 10:38 AM · Restricted Project