Page MenuHomePhabricator

dblaikie (David Blaikie)
User

Projects

User Details

User Since
Oct 8 2012, 9:19 AM (328 w, 2 d)

Recent Activity

Today

dblaikie added a comment to D57115: [llvm] Remove dependency on <algorithm> [NFC].

"mainly" used for std::sort might be true, but "only" used might be harder to prove - did you use any tooling to test that these inclusions weren't needed other than "this code still compiles" (which has the problem that the inclusion might be used, but also included indirectly from some other header - so removing it may still compile, but introduces an undesirable indirect dependency on an implementation detail of some other header)

I ran ninja check-all and did not see any failures. I haven't changed the places where algorithm is needed (like for std::fill, etc) and which introduced compile errors. I have also not touched examples, unittests and util/unittest directories.

How did you identify these "I haven't changed the places where algorithm is needed (like for std::fill, etc)" places? By visual inspection of every file that's been touched?

No, I removed the header from all files and then put back only where the compilation failed. I agree with your comment "removing it may still compile, but introduces an undesirable indirect dependency on an implementation detail of some other header".

Wed, Jan 23, 1:00 PM
dblaikie added a comment to D57115: [llvm] Remove dependency on <algorithm> [NFC].

"mainly" used for std::sort might be true, but "only" used might be harder to prove - did you use any tooling to test that these inclusions weren't needed other than "this code still compiles" (which has the problem that the inclusion might be used, but also included indirectly from some other header - so removing it may still compile, but introduces an undesirable indirect dependency on an implementation detail of some other header)

I ran ninja check-all and did not see any failures. I haven't changed the places where algorithm is needed (like for std::fill, etc) and which introduced compile errors. I have also not touched examples, unittests and util/unittest directories.

Wed, Jan 23, 12:47 PM
dblaikie added a comment to D57115: [llvm] Remove dependency on <algorithm> [NFC].

"mainly" used for std::sort might be true, but "only" used might be harder to prove - did you use any tooling to test that these inclusions weren't needed other than "this code still compiles" (which has the problem that the inclusion might be used, but also included indirectly from some other header - so removing it may still compile, but introduces an undesirable indirect dependency on an implementation detail of some other header)

Wed, Jan 23, 12:41 PM
dblaikie added a comment to D49426: [DEBUG_INFO] fix .loc directives emitted for missing prologues.

I'm OK with this change from a "slightly reduces debug info size" but I hesitate to suggest that this isn't a thing your consumer should support & this "When an assembly file with multiple .loc directives with no interveaning instructions are assembled, GAS produces a DWARF line table that maps one instruction to multiple entries." sounds like a bit of a mischaracterization of how I'd read the line table entries in question - I think this line table doesn't "map one instruction to multiple entries" but instead has an empty entry that contains no instructions. Everything between one .loc and the next is at that first location - if there's nothing between them, then nothing is at that location. I suspect that's how other consumers are viewing this situation.

Wed, Jan 23, 11:54 AM
dblaikie accepted D56220: [llvm] Clarify responsiblity of some of DILocation discriminator APIs.

Seems good - thanks!

Wed, Jan 23, 11:46 AM
dblaikie added a comment to D53329: Generate DIFile with main program if source is not available.

Thanks for filing the bug/reducing a test case - this change should include an automated test case that captures this issue (check for other tests that pass -gembed-source to see how this might be tested, possibly expanding the existing test case case or introducing a new one (test/CodeGen/debug-info-embed-source.c looks like the place to start)

Wed, Jan 23, 11:45 AM · debug-info

Yesterday

dblaikie added a comment to D57023: [MsgPack] New MsgPackDocument class.

Probably best to have someone else from AMDGPU land review this for your uses there - I'm just giving some high level at-a-glance stylistic coverage here.

Tue, Jan 22, 1:18 PM

Sun, Jan 13

dblaikie added inline comments to D56435: We can improve the performance (generally) by memo-izing the action to map a debug location to its function summary..
Sun, Jan 13, 6:20 PM
dblaikie added inline comments to D55394: Re-order type param children of ObjC nodes.
Sun, Jan 13, 5:17 PM

Tue, Jan 8

dblaikie added a comment to D56438: [Modules] Allow modulemap path change for implicitly built modules.

No doubt @rsmith will have a more nuanced/accurate opinion on this than I do, but I would've thought the point of implicit modules is that they don't get moved around & it sounds like that's what this patch is suggesting/supporting, but maybe I haven't understood it fully?

Tue, Jan 8, 3:35 PM
dblaikie added inline comments to rL350070: Use error() instead of fatal() to report an invalid address range..
Tue, Jan 8, 2:20 AM

Sun, Jan 6

dblaikie added a comment to D56245: Use llvm_unreachable instead of errs+abort in MCStreamer.cpp (in EmitRawTextImpl). NFC..
In D56245#1345316, @rnk wrote:

I don't think this should be llvm_unreachable, I think it should be report_fatal_error. LLVM has some some support for out of tree backends, and they could plausibly call this when using a release build of LLVM as a library. If all the callers were in-tree, then calling this would represent an LLVM-internal logic error (forgetting to check hasRawTextSupport()), and unreachable or assert would be appropriate.

Sun, Jan 6, 9:52 PM

Dec 24 2018

dblaikie committed rL350050: Fix build - follow-up to r350048 which broke headerless (v4) address pool.
Fix build - follow-up to r350048 which broke headerless (v4) address pool
Dec 24 2018, 12:00 AM

Dec 23 2018

dblaikie committed rL350048: DebugInfo: Use assembly label arithmetic for address pool size for easier….
DebugInfo: Use assembly label arithmetic for address pool size for easier…
Dec 23 2018, 11:38 PM
dblaikie committed rL350047: DebugInfo: Add assembly comments for debug_addr contribution header fields.
DebugInfo: Add assembly comments for debug_addr contribution header fields
Dec 23 2018, 11:13 PM
dblaikie committed rL350046: llvm-dwarfdump: Skip address index info (and dump only the address, if found)….
llvm-dwarfdump: Skip address index info (and dump only the address, if found)…
Dec 23 2018, 10:56 PM

Dec 22 2018

dblaikie committed rLLD350011: Test DWARFv5 with gdb-index and low_pc/high_pc on the CU (rather than ranges).
Test DWARFv5 with gdb-index and low_pc/high_pc on the CU (rather than ranges)
Dec 22 2018, 2:26 PM
dblaikie committed rL350011: Test DWARFv5 with gdb-index and low_pc/high_pc on the CU (rather than ranges).
Test DWARFv5 with gdb-index and low_pc/high_pc on the CU (rather than ranges)
Dec 22 2018, 2:24 PM
dblaikie committed rL350010: DebugInfo: Accurately propagate the section used by a relocation when accessing….
DebugInfo: Accurately propagate the section used by a relocation when accessing…
Dec 22 2018, 2:24 PM
dblaikie committed rL350009: llvm-dwarfdump: Dump the section name/number for addr attributes.
llvm-dwarfdump: Dump the section name/number for addr attributes
Dec 22 2018, 12:38 PM
dblaikie committed rL349997: llvm-dwarfdump: Remove extraneous space between '(' and 'indexed'.
llvm-dwarfdump: Remove extraneous space between '(' and 'indexed'
Dec 22 2018, 12:46 AM
dblaikie committed rL349996: llvm-dwarfdump: Print the section name/number for addr_index attributes.
llvm-dwarfdump: Print the section name/number for addr_index attributes
Dec 22 2018, 12:37 AM
dblaikie committed rL349995: DebugInfo: Refactor named section dumping into a reusable helper.
DebugInfo: Refactor named section dumping into a reusable helper
Dec 22 2018, 12:28 AM

Dec 21 2018

dblaikie committed rL349985: DebugInfo: Remove extra attribute lookup.
DebugInfo: Remove extra attribute lookup
Dec 21 2018, 6:27 PM
dblaikie committed rLLD349979: gdb-index: Handle errors when parsing ranges.
gdb-index: Handle errors when parsing ranges
Dec 21 2018, 4:34 PM
dblaikie committed rL349979: gdb-index: Handle errors when parsing ranges.
gdb-index: Handle errors when parsing ranges
Dec 21 2018, 4:34 PM
dblaikie committed rL349978: libDebugInfo: Refactor error handling in range list parsing.
libDebugInfo: Refactor error handling in range list parsing
Dec 21 2018, 4:34 PM
dblaikie committed rL349968: Reapply: DebugInfo: Assume an absence of ranges or high_pc on a CU means the CU….
Reapply: DebugInfo: Assume an absence of ranges or high_pc on a CU means the CU…
Dec 21 2018, 2:29 PM
dblaikie accepted D55681: [llvm] API for encoding/decoding DWARF discriminators..

Not sure I'm following this all in detail & at least for me (someone not super familiar with the details here) it might've been a bit easier to review in smaller pieces (breaking up the refactoring to generalize the components of the discriminator into smaller bits, etc) but all good. Thanks!

Dec 21 2018, 2:05 PM

Dec 20 2018

dblaikie committed rL349819: DebugInfo: Fix for missing comp_dir handling with r349207.
DebugInfo: Fix for missing comp_dir handling with r349207
Dec 20 2018, 12:50 PM

Dec 19 2018

dblaikie committed rL349670: llvm-dwarfdump: Improve/fix pretty printing of array dimensions.
llvm-dwarfdump: Improve/fix pretty printing of array dimensions
Dec 19 2018, 11:37 AM
dblaikie closed D55721: llvm-dwarfdump: Improve/fix pretty printing of array dimensions.
Dec 19 2018, 11:37 AM
dblaikie committed rC349669: PR40096: Forwards-compatible with C++20 rule regarding aggregates not having….
PR40096: Forwards-compatible with C++20 rule regarding aggregates not having…
Dec 19 2018, 11:37 AM
dblaikie committed rL349669: PR40096: Forwards-compatible with C++20 rule regarding aggregates not having….
PR40096: Forwards-compatible with C++20 rule regarding aggregates not having…
Dec 19 2018, 11:37 AM
dblaikie added a comment to D55681: [llvm] API for encoding/decoding DWARF discriminators..

Might be worth having a summary of the goals here (rather than only by reference to the design discussion thread) for review and later as commit history, etc.

Dec 19 2018, 11:23 AM

Dec 18 2018

dblaikie added inline comments to D55721: llvm-dwarfdump: Improve/fix pretty printing of array dimensions.
Dec 18 2018, 2:36 PM
dblaikie updated the diff for D55721: llvm-dwarfdump: Improve/fix pretty printing of array dimensions.

Rename dumpArray to dumpArrayType
Implement & test behavior for subrange without any attributes (no low/high/count).

Dec 18 2018, 2:36 PM
dblaikie updated the diff for D55721: llvm-dwarfdump: Improve/fix pretty printing of array dimensions.

Pull out array printing into a helper function

Dec 18 2018, 11:48 AM
dblaikie committed rL349528: DebugInfo: Fix missing local imported entities after r349207.
DebugInfo: Fix missing local imported entities after r349207
Dec 18 2018, 11:45 AM
dblaikie added a comment to D55281: debuginfo: Use symbol difference for CU length to simplify assembly reading/editing.

I'm just still not sure the extra cl::opt is worth it, though - are you? Is the extra coverage significant/important?

I'd rather see a test break locally (because it's target-agnostic and uses the flag) than wait for some NVPTX-target bot to break after I commit.

Dec 18 2018, 9:25 AM
dblaikie added a comment to D55281: debuginfo: Use symbol difference for CU length to simplify assembly reading/editing.

Still a bit curious about the other discussion about flags, etc.

Well, you have better access to Eric than the rest of us....

Dec 18 2018, 8:44 AM
dblaikie added a comment to D55721: llvm-dwarfdump: Improve/fix pretty printing of array dimensions.

Note that the lower-bound might be implicit, but it's never "unknown."
We do need to account for the cases where the implicit lower-bound is 1. The Fortran people will need this, and the OpenVMS folks support several other languages that also have a default LB of 1.

Using [C] (C = count) for both the count-only and upper-bound-only cases suggests that we're being descriptive rather than carefully reflecting the attributes that are in the subrange DIE, which is fine. So, if an explicit LB matches the language default, then the display should also be [C] (where C = count if we have a count, or C = U + (1 - L) if we have upper_bound).

The only change you're suggesting is to detect when an explicit LB matches the language default, and treating that case the same as if the LB were not specified (in terms of how it's printed out)?

Sorry, no, in the course of editing the comment I lost the important part: When converting UB-only to Count, you need to correctly factor in the default LB: C = UB + (1 - DefaultLBForLanguage) otherwise the display for 1-based languages will be incorrect.

Then, since you have to know the default LB anyway, the rest seems not too hard.

Dec 18 2018, 8:42 AM
dblaikie updated the diff for D55721: llvm-dwarfdump: Improve/fix pretty printing of array dimensions.

Use language default lower bound to inform size calculation from upper_bound with no lower bound
Use [size] rendering when explicit lower bound matches language default

Dec 18 2018, 8:39 AM
dblaikie added inline comments to D55760: [ADT] IntervalMap: add contains(a, b) method.
Dec 18 2018, 7:46 AM

Dec 17 2018

dblaikie added a comment to D55281: debuginfo: Use symbol difference for CU length to simplify assembly reading/editing.

Recommitted this, predicating everything on !useSectionsAsReferences, which means the existing sections_as_references passes unmodified & I added another separate test to validate the change. Thanks again @ABataev for that pointer/fix! (also validated it against the internal failure that caught this too)

Dec 17 2018, 5:10 PM
dblaikie committed rL349430: Recommit r348806: DebugInfo: Use symbol difference for CU length to simplify….
Recommit r348806: DebugInfo: Use symbol difference for CU length to simplify…
Dec 17 2018, 5:09 PM
dblaikie added a comment to D55281: debuginfo: Use symbol difference for CU length to simplify assembly reading/editing.

@aprantl @probinson - you folks haev any feelings about this? Looks like these flags are essentially unused (except for testing, which could equally be accomplished by tests that specify an NVPTX triple, I think (except on builds that aren't building an NVPTX backend)) & are a collection of features that are all only needed on NVPTX. I'd lean towards removing them all & gating this on NVPTX. Or on one flag rather than several fine-grained ones, if/when someone starts playing with a ptx assembler that can cope with these features.

But yeah, no big deal I guess - leads to a bit of confusion about who these flags are meant for, under what conditions some of these features might be enabled but not others, etc.

In a way this reminds me of the discussions around how to do "debugger tuning" and there were some very strong voices in favor of unpacking the tuning setting into separate flags. I believe that the DwarfDebug ctor is the only place that directly checks tuning, or that was the goal on the LLVM side anyhow.

Dec 17 2018, 4:31 PM
dblaikie added a comment to D55281: debuginfo: Use symbol difference for CU length to simplify assembly reading/editing.

But I'm still confused & would love some help understanding why/how some of these features are gated by the cl::opt -dwarf-sections-as-references or NVPTX - but other features are gated only on NVPTX? Should we remove -dwarf-sections-as-references and just rely on NVPTX for all these things? Or is there some other use of -dwarf-sections-as-references that isn't NVPTX?

Because we don't want to rely on a particular target type. Yes, currently it is used only for NVPTX. But later there might be some other targets with the same limitation and we can reuse the same option for all such targets.

But all the other things (ranges, inline strings, loc, etc) are predicated based on NVPTX directly, right? Seeing one part of it separated out as a flag makes me think that there are other users of that flag - but if they're all equally requirements/limitations of ptxas, seems to me they should be predicated/treated the same way (& if we had another user that ends up needing some of this, we could refactor that out/flag it at that point - and then the difference between the handling would be explained by the difference in the needs of the current NVPTX situation and whatever the new thing is?)

No, most of them are also controlled by the options like -dwarf-inlined-strings for inlined strings. They are just forced to et to true by default for NVPTX.

Fair enough - though I'm not really sure of the value of all the flags.

@aprantl @probinson - you folks haev any feelings about this? Looks like these flags are essentially unused (except for testing, which could equally be accomplished by tests that specify an NVPTX triple, I think (except on builds that aren't building an NVPTX backend)) & are a collection of features that are all only needed on NVPTX. I'd lean towards removing them all & gating this on NVPTX. Or on one flag rather than several fine-grained ones, if/when someone starts playing with a ptx assembler that can cope with these features.

But yeah, no big deal I guess - leads to a bit of confusion about who these flags are meant for, under what conditions some of these features might be enabled but not others, etc.

David, all the patches that introduced those options were reviewed and accepted by Eric Christopher.

Dec 17 2018, 2:32 PM
dblaikie added a comment to D55281: debuginfo: Use symbol difference for CU length to simplify assembly reading/editing.

But I'm still confused & would love some help understanding why/how some of these features are gated by the cl::opt -dwarf-sections-as-references or NVPTX - but other features are gated only on NVPTX? Should we remove -dwarf-sections-as-references and just rely on NVPTX for all these things? Or is there some other use of -dwarf-sections-as-references that isn't NVPTX?

Because we don't want to rely on a particular target type. Yes, currently it is used only for NVPTX. But later there might be some other targets with the same limitation and we can reuse the same option for all such targets.

But all the other things (ranges, inline strings, loc, etc) are predicated based on NVPTX directly, right? Seeing one part of it separated out as a flag makes me think that there are other users of that flag - but if they're all equally requirements/limitations of ptxas, seems to me they should be predicated/treated the same way (& if we had another user that ends up needing some of this, we could refactor that out/flag it at that point - and then the difference between the handling would be explained by the difference in the needs of the current NVPTX situation and whatever the new thing is?)

No, most of them are also controlled by the options like -dwarf-inlined-strings for inlined strings. They are just forced to et to true by default for NVPTX.

Dec 17 2018, 1:57 PM
dblaikie added a comment to D55281: debuginfo: Use symbol difference for CU length to simplify assembly reading/editing.

But I'm still confused & would love some help understanding why/how some of these features are gated by the cl::opt -dwarf-sections-as-references or NVPTX - but other features are gated only on NVPTX? Should we remove -dwarf-sections-as-references and just rely on NVPTX for all these things? Or is there some other use of -dwarf-sections-as-references that isn't NVPTX?

Because we don't want to rely on a particular target type. Yes, currently it is used only for NVPTX. But later there might be some other targets with the same limitation and we can reuse the same option for all such targets.

Dec 17 2018, 1:35 PM
dblaikie added a comment to D55281: debuginfo: Use symbol difference for CU length to simplify assembly reading/editing.

Hey Alexey - thanks for taking a look!

NVPTX does not support labels in the debug info sections. All these new labels must be created/emitted only if DwarfDebug::useSectionsAsReferences() is false.

Is that the right name for the flag? If you reckon so - though I'm not sure it means a lot to me, personally. (

Also, a few places in lib/CodeGen/AsmPrinter/* use isNVPTX directly, not UseSectionsAsReferences - are they correct? (eg: UseRangeSection, UseInlineStrings, UseLocSection, some test for physical registers in updateSubprogramScopeDIE) What's the difference between these two things?

useSectionsAsReferences() does not allow to define inner symbols in the debug sections and force using the section names itself as references in other debug info sections (if it is supported). You need to use this one. Other flags are also required for the NVPTX debug info, but they control different format features.

Fair enough & I'll work on that (want to reproduce the internal failure first to then test this approach works there too)

But I'd still like to understand the difference between these choices - given Google encountered an internal failure where testing NVPTX was insufficient but it looked like it was still related to LLVM's assembly being passed to ptxas - wouldn't these other conditions be incorrect in that case? (ie: assuming there's some non-NVPTX target that uses ptxas, and anything using ptxas needs to disable all these features (ranges, inline strings, etc, etc) - I would've thought these cases testing NVPTX directly would be bugs? Or are there different/varied reasons these things are disabled in some situations but not others?)

I need to make those tests more strict. Currently, they are using CHECK in all places, though we need CHECK-NEXT to catch the situation you ran into. It seems to me, you emitted the label somewhere in the debug info sections and the checks for NVPTX debug info missed this label.

Sure - sorry, I'm still trying to understand this better.

Targeting NVPTX itself enables the "UseSectionsAsReferences" as well as disabling UseRangeSection, UseInlineStrings, UseLocSection, etc...

I made made the change only on !NVPTX, but that seemed to be insufficient - some internal test failed anyway (so it was !NVPTX and still using ptxas, I guess?)

What I'm trying to understand is, if such a thing (ptxas but not NVPTX) - how does it function? Since it would not do the other NVPTX things like disabling UseRangeSection, UseInlineStrings, and UesLocSection.

Does that make sense as a question? Is there an answer?

Not all of your changes are guarded by !NVPTX. E.g. Asm->OutStreamer->EmitLabel(BeginLabel); is not, but it emits the label inside of the debug info section. ptxas does not support labels at all! It uses oversimplified DWARF2, where the references can be emitted only as section_name +- offset.
All new EmitLabel()s must be guarded by !UseSectionsAsReferences condition.
Also, you can manually run the tests for the NVPTX debug info and you will definitely find all that new emitted labels in the output. You need to ensure, that your changes do not add new labels in the output when UseSectionsAsReferences is set to true.

Dec 17 2018, 1:24 PM
dblaikie committed rL349395: DebugInfo: Update gold plugin tests due to CU attribute reordering in r349207.
DebugInfo: Update gold plugin tests due to CU attribute reordering in r349207
Dec 17 2018, 1:13 PM
dblaikie added a comment to D55281: debuginfo: Use symbol difference for CU length to simplify assembly reading/editing.

Hey Alexey - thanks for taking a look!

NVPTX does not support labels in the debug info sections. All these new labels must be created/emitted only if DwarfDebug::useSectionsAsReferences() is false.

Is that the right name for the flag? If you reckon so - though I'm not sure it means a lot to me, personally. (

Also, a few places in lib/CodeGen/AsmPrinter/* use isNVPTX directly, not UseSectionsAsReferences - are they correct? (eg: UseRangeSection, UseInlineStrings, UseLocSection, some test for physical registers in updateSubprogramScopeDIE) What's the difference between these two things?

useSectionsAsReferences() does not allow to define inner symbols in the debug sections and force using the section names itself as references in other debug info sections (if it is supported). You need to use this one. Other flags are also required for the NVPTX debug info, but they control different format features.

Fair enough & I'll work on that (want to reproduce the internal failure first to then test this approach works there too)

But I'd still like to understand the difference between these choices - given Google encountered an internal failure where testing NVPTX was insufficient but it looked like it was still related to LLVM's assembly being passed to ptxas - wouldn't these other conditions be incorrect in that case? (ie: assuming there's some non-NVPTX target that uses ptxas, and anything using ptxas needs to disable all these features (ranges, inline strings, etc, etc) - I would've thought these cases testing NVPTX directly would be bugs? Or are there different/varied reasons these things are disabled in some situations but not others?)

I need to make those tests more strict. Currently, they are using CHECK in all places, though we need CHECK-NEXT to catch the situation you ran into. It seems to me, you emitted the label somewhere in the debug info sections and the checks for NVPTX debug info missed this label.

Dec 17 2018, 1:10 PM
dblaikie added a comment to D55281: debuginfo: Use symbol difference for CU length to simplify assembly reading/editing.

Hey Alexey - thanks for taking a look!

NVPTX does not support labels in the debug info sections. All these new labels must be created/emitted only if DwarfDebug::useSectionsAsReferences() is false.

Is that the right name for the flag? If you reckon so - though I'm not sure it means a lot to me, personally. (

Also, a few places in lib/CodeGen/AsmPrinter/* use isNVPTX directly, not UseSectionsAsReferences - are they correct? (eg: UseRangeSection, UseInlineStrings, UseLocSection, some test for physical registers in updateSubprogramScopeDIE) What's the difference between these two things?

useSectionsAsReferences() does not allow to define inner symbols in the debug sections and force using the section names itself as references in other debug info sections (if it is supported). You need to use this one. Other flags are also required for the NVPTX debug info, but they control different format features.

Dec 17 2018, 1:03 PM
dblaikie added a comment to D55281: debuginfo: Use symbol difference for CU length to simplify assembly reading/editing.

Hey Alexey - thanks for taking a look!

Dec 17 2018, 12:49 PM
dblaikie added inline comments to D55760: [ADT] IntervalMap: add contains(a, b) method.
Dec 17 2018, 10:58 AM
dblaikie added a comment to D55770: [clangd] BackgroundIndex rebuilds symbol index periodically..

(from the peanut gallery): Usually what I'd expect here is "reindexing will occur X milliseconds after a write, covering anything written up until that point" - so it doesn't reindex unnecessarily if the files are unmodified/sitting idle, and it doesn't thrash reindexing for multiple changes in rapid succession (such as hitting the "save all" button in some text editor - or having made a few other quick changes one after another maybe), but still provides reasonable up-to-date data for the user.

Dec 17 2018, 10:56 AM
dblaikie accepted D55762: [llvm-dwarfdump] - Do not error out on R_X86_64_DTPOFF64/R_X86_64_DTPOFF32 relocations..

Sounds alright to me - thanks!

Dec 17 2018, 10:53 AM
dblaikie committed rL349333: DebugInfo: Assume an absence of ranges or high_pc on a CU means the CU is empty….
DebugInfo: Assume an absence of ranges or high_pc on a CU means the CU is empty…
Dec 17 2018, 12:30 AM

Dec 14 2018

dblaikie added a comment to D55721: llvm-dwarfdump: Improve/fix pretty printing of array dimensions.

Note that the lower-bound might be implicit, but it's never "unknown."
We do need to account for the cases where the implicit lower-bound is 1. The Fortran people will need this, and the OpenVMS folks support several other languages that also have a default LB of 1.

Using [C] (C = count) for both the count-only and upper-bound-only cases suggests that we're being descriptive rather than carefully reflecting the attributes that are in the subrange DIE, which is fine. So, if an explicit LB matches the language default, then the display should also be [C] (where C = count if we have a count, or C = U + (1 - L) if we have upper_bound).

Dec 14 2018, 3:34 PM
dblaikie committed rL349207: DebugInfo: Avoid using split DWARF when the split unit would be empty..
DebugInfo: Avoid using split DWARF when the split unit would be empty.
Dec 14 2018, 2:48 PM
dblaikie committed rL349203: DebugInfo: Move addAddrBase from DwarfUnit to DwarfCompileUnit.
DebugInfo: Move addAddrBase from DwarfUnit to DwarfCompileUnit
Dec 14 2018, 2:37 PM
dblaikie created D55721: llvm-dwarfdump: Improve/fix pretty printing of array dimensions.
Dec 14 2018, 2:21 PM

Dec 12 2018

dblaikie committed rL348974: Fix for llvm-dwarfdump changes for subroutine types.
Fix for llvm-dwarfdump changes for subroutine types
Dec 12 2018, 1:19 PM
dblaikie added a comment to D36993: [llvm-dwarfdump] Print type names in DW_AT_type DIEs.

FWIW, I ended up adding test coverage for some of this in r348954 while improving the printing of arrays to include array dimensions - hand written assembly, so yeah, it's a bit less than ideal to maintain (mostly having to edit both the abbreviation and the definition - a solution to that would be handy (& having to lookup the numeric values of the enums (DW_AT/DW_TAGs))) but I think it's worth having.

I can imagine some scheme involving a tool that generates assembler-source constants by pulling in Dwarf.def, but it's harder to imagine getting that hooked into the test infrastructure somehow.

Dec 12 2018, 1:11 PM
dblaikie committed rL348965: DebugInfo/DWARF: Pretty print subroutine types.
DebugInfo/DWARF: Pretty print subroutine types
Dec 12 2018, 11:57 AM
dblaikie committed rL348962: DebugInfo/DWARF: Improve dumping of pointers to members ('int foo::*' rather….
DebugInfo/DWARF: Improve dumping of pointers to members ('int foo::*' rather…
Dec 12 2018, 11:37 AM
dblaikie committed rL348961: DebugInfo/DWARF: Refactor type dumping to dump types, rather than DIEs that….
DebugInfo/DWARF: Refactor type dumping to dump types, rather than DIEs that…
Dec 12 2018, 11:36 AM
dblaikie committed rL348960: DebugInfo/DWARF: Refactor getAttributeValueAsReferencedDie to accept a….
DebugInfo/DWARF: Refactor getAttributeValueAsReferencedDie to accept a…
Dec 12 2018, 11:27 AM
dblaikie committed rLLD348955: Update for an llvm-dwarfdump change in output.
Update for an llvm-dwarfdump change in output
Dec 12 2018, 10:53 AM
dblaikie committed rL348955: Update for an llvm-dwarfdump change in output.
Update for an llvm-dwarfdump change in output
Dec 12 2018, 10:52 AM
dblaikie committed rL348954: llvm-dwarfdump: Dump array dimensions in stringified type names.
llvm-dwarfdump: Dump array dimensions in stringified type names
Dec 12 2018, 10:52 AM
dblaikie added a comment to D36993: [llvm-dwarfdump] Print type names in DW_AT_type DIEs.

FWIW, I ended up adding test coverage for some of this in r348954 while improving the printing of arrays to include array dimensions - hand written assembly, so yeah, it's a bit less than ideal to maintain (mostly having to edit both the abbreviation and the definition - a solution to that would be handy (& having to lookup the numeric values of the enums (DW_AT/DW_TAGs))) but I think it's worth having.

Dec 12 2018, 10:52 AM

Dec 10 2018

dblaikie committed rL348814: Follow-up fix to r348811 for null Errors (which is the case for end iterators).
Follow-up fix to r348811 for null Errors (which is the case for end iterators)
Dec 10 2018, 4:20 PM
dblaikie committed rL348811: llvm-objcopy: Improve/simplify llvm::Error handling during notes iteration.
llvm-objcopy: Improve/simplify llvm::Error handling during notes iteration
Dec 10 2018, 4:12 PM
dblaikie closed D55235: llvm-objcopy: Improve/simplify llvm::Error handling during notes iteration.
Dec 10 2018, 4:12 PM
dblaikie committed rL348806: debuginfo: Use symbol difference for CU length to simplify assembly….
debuginfo: Use symbol difference for CU length to simplify assembly…
Dec 10 2018, 2:48 PM
dblaikie closed D55281: debuginfo: Use symbol difference for CU length to simplify assembly reading/editing.
Dec 10 2018, 2:48 PM
dblaikie added a comment to D55234: Do not use a hash table to uniquify mergeable strings..

I think you should focus on large programs. We don't really care about the marginal improvements or regressions for small programs because linking small program is very fast anyway, and we don't care about 10ms improvements or regressions. For large programs, it seems reasonably positive. Also this patch only deletes code.

FWIW linking times on smaller (than the very large) programs like Clang are still significant to many folks (including LLVM developers themselves) - for example when running "ninja check-all" many LLVM tools (~30? I forget roughly how many) have to be linked - and saving time on each (or the longest one) means getting out of that very serial step in the check run (massively parallel compiles, serial links, then massively parallel test runs).

I second the point, but see my comment above. If we want to optimize further, I think we should try some one-entry cache and leverage the getVA() calling pattern, instead of using a hash table (which has been deleted by this patch). The hash table may actually do worse with some SHF_MERGE|SHF_STRINGS.

Dec 10 2018, 10:51 AM
dblaikie added a comment to D55234: Do not use a hash table to uniquify mergeable strings..

I think you should focus on large programs. We don't really care about the marginal improvements or regressions for small programs because linking small program is very fast anyway, and we don't care about 10ms improvements or regressions. For large programs, it seems reasonably positive. Also this patch only deletes code.

Dec 10 2018, 10:35 AM

Dec 9 2018

dblaikie added inline comments to D55468: Use zip_longest for iterator range comparisons. NFC..
Dec 9 2018, 4:30 PM
dblaikie accepted D55468: Use zip_longest for iterator range comparisons. NFC..

Overall I'm not sure this construct/pattern improves readability, but not too fussed either way.

Dec 9 2018, 3:27 PM
dblaikie added inline comments to D55468: Use zip_longest for iterator range comparisons. NFC..
Dec 9 2018, 1:40 PM
dblaikie added inline comments to D55468: Use zip_longest for iterator range comparisons. NFC..
Dec 9 2018, 1:07 PM

Dec 7 2018

dblaikie added inline comments to D55235: llvm-objcopy: Improve/simplify llvm::Error handling during notes iteration.
Dec 7 2018, 5:03 PM
dblaikie added inline comments to D55235: llvm-objcopy: Improve/simplify llvm::Error handling during notes iteration.
Dec 7 2018, 3:21 PM

Dec 5 2018

dblaikie committed rL348416: ThinLTO: Do not import debug info for imported global constants.
ThinLTO: Do not import debug info for imported global constants
Dec 5 2018, 1:45 PM
dblaikie closed D55309: ThinLTO: Do not import debug info for imported global constants.
Dec 5 2018, 1:45 PM
dblaikie added a comment to D55309: ThinLTO: Do not import debug info for imported global constants.

From what I'm seeing, I would imagine this may regress debug info quality a bit. In the narrow example given, the DWARF still ends up with a description of 'foo', but without any location/value - though this is the same as if 'foo' was in the same translation unit (looks like LLVM fails to track the removing of the global and replacing everything with a constant - I believe the debug info metadata supports describing such a situation, but it isn't being used here - with or without this new change). SO that's not a regression.

Dec 5 2018, 11:09 AM
dblaikie added a comment to D55309: ThinLTO: Do not import debug info for imported global constants.

Could you tell me the commands I need to run to test this?

You can try:

clang -flto=thin -O3 main.c foo.c -fuse-ld=lld
Dec 5 2018, 9:36 AM
dblaikie added a comment to D55309: ThinLTO: Do not import debug info for imported global constants.

Hello David,

This is an example of importing read-only GV:

// main.c
extern int foo;
int main() { return foo; }
// foo.c
int foo = 42;

The variable foo is imported to main TU and internalized.

Dec 5 2018, 8:26 AM
dblaikie accepted D55261: [llvm-dwarfdump] - Simplify the test case..

Great, thanks!

Dec 5 2018, 8:23 AM

Dec 4 2018

dblaikie added a comment to D55309: ThinLTO: Do not import debug info for imported global constants.

Ah, I found a comment from the original review:

Dec 4 2018, 7:01 PM
dblaikie updated the summary of D55309: ThinLTO: Do not import debug info for imported global constants.
Dec 4 2018, 6:53 PM
dblaikie added a comment to D55309: ThinLTO: Do not import debug info for imported global constants.

I'm unaware of any regression for debug info quality that occurs with this change, but I haven't done wide scale testing of it - does anyone have a deeper/practical/feature test that demonstrates the motivation for this part of the change in the first place?

Dec 4 2018, 6:50 PM
dblaikie created D55309: ThinLTO: Do not import debug info for imported global constants.
Dec 4 2018, 6:49 PM
dblaikie added inline comments to D51183: [clang-query] Continue if compilation command not found for some files.
Dec 4 2018, 6:14 PM
dblaikie removed a reviewer for D51183: [clang-query] Continue if compilation command not found for some files: dblaikie.
Dec 4 2018, 12:22 PM
dblaikie added a comment to D55281: debuginfo: Use symbol difference for CU length to simplify assembly reading/editing.

What happens today with NVPTX for other debug info sections that use label arithmetic? I know for sure that the accelerator tables/debug_names is using this, but maybe they're not used with this target?

Dec 4 2018, 10:23 AM
dblaikie added a comment to D55281: debuginfo: Use symbol difference for CU length to simplify assembly reading/editing.

This change is not compatible with NVPTX, it does not support any of the label arithmetics.

Dec 4 2018, 10:20 AM