Page MenuHomePhabricator

dblaikie (David Blaikie)
User

Projects

User Details

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

Recent Activity

Yesterday

dblaikie added a comment to D96045: [llvm-dwarfdump][locstats] Unify handling of inlined vars with no loc.

After moving the variable InlinedFnsToBeProcessed to inside the for loop, I noticed a difference in two dwarfdump statistics for a quite large binary with fission=yes:

#variables processed by location statistics
#variables with 0% of parent scope covered by DW_AT_location

Since I am not very familiar with this code, I would request if you could investigate and apply the patch.

Mon, Apr 12, 5:42 PM · debug-info, Restricted Project
dblaikie accepted D100137: [JumpThreading] merge debug info when merging select+br.

Perfect - thanks a lot!

Mon, Apr 12, 5:37 PM · debug-info, Restricted Project
dblaikie requested review of D100353: Support nodebug in SCCP.
Mon, Apr 12, 5:15 PM · Restricted Project
dblaikie added a comment to D100137: [JumpThreading] merge debug info when merging select+br.

Since https://reviews.llvm.org/D100158 is approved, are we still interested in this patch fixing what looks to me like a loss of debug info?

Mon, Apr 12, 4:52 PM · debug-info, Restricted Project
dblaikie accepted D100158: [SantizerCoverage] handle missing DBG MD when inserting libcalls.

Looks good - thanks!

Mon, Apr 12, 3:38 PM · Restricted Project
dblaikie added inline comments to D100158: [SantizerCoverage] handle missing DBG MD when inserting libcalls.
Mon, Apr 12, 1:55 PM · Restricted Project

Sat, Apr 10

dblaikie added inline comments to D100159: Support: Add move semantics to mapped_file_region.
Sat, Apr 10, 11:25 AM · Restricted Project

Fri, Apr 9

dblaikie added inline comments to D100158: [SantizerCoverage] handle missing DBG MD when inserting libcalls.
Fri, Apr 9, 5:46 PM · Restricted Project
dblaikie accepted D100159: Support: Add move semantics to mapped_file_region.

Looks great!

Fri, Apr 9, 1:40 PM · Restricted Project
dblaikie added a comment to D100158: [SantizerCoverage] handle missing DBG MD when inserting libcalls.

My best guess would be that any call synthesized from an instruction without a location should probably just get a zero line scoped directly to the current function.

Sure. Should we do that unconditionally for the inserted CallInst, or predicate it on debug info being requested for the compilation, or something else?

Fri, Apr 9, 11:56 AM · Restricted Project

Thu, Apr 8

dblaikie added a comment to D100137: [JumpThreading] merge debug info when merging select+br.

I guess it'd probably be worth fixing that code to do something to add a valid location to those function calls too

Yeah, that sounds like the best long term solution to me; otherwise we'll be playing whack-a-mole with destructive transforms losing debug and results in confusing-for-developers error messages. When splitting a critical edge, what location do we use if neither contain debug info? If only one is missing debug info, I'd guess that Instruction::applyMergedLocation will pick the one existing one?

Thu, Apr 8, 8:16 PM · debug-info, Restricted Project
dblaikie added a comment to D100158: [SantizerCoverage] handle missing DBG MD when inserting libcalls.

My best guess would be that any call synthesized from an instruction without a location should probably just get a zero line scoped directly to the current function. It's not ideal (not only the lack of location, but it also punches holes in scopes which can bloat debug info/make stepping more difficult as you hit discontiguous hunks of lines potentially, etc)

Thu, Apr 8, 8:07 PM · Restricted Project
dblaikie added inline comments to D98218: [LSR] fix a issue that LoopStrengthReduction drop debug location unnecessarily.
Thu, Apr 8, 7:27 PM · Restricted Project
dblaikie added a comment to D100159: Support: Add move semantics to mapped_file_region.

looks like the underlying mapped_file_region could be made movable without the overhead of Optional (both the Windows and Unix dtors of mapped_file_region test whether the mapping is non-null, so a move ctor can null out the Mapping member to ensure no double-closing, etc) which might be nicer (avoiding the Optional overhead)/more general?

Thu, Apr 8, 7:27 PM · Restricted Project
dblaikie committed rGeb8a28e2cf03: DebugInfo: Include inline namespaces in template specialization parameter names (authored by dblaikie).
DebugInfo: Include inline namespaces in template specialization parameter names
Thu, Apr 8, 5:50 PM
dblaikie committed rG8294019633b5: Use default ref capture to avoid unused capture warning on assert-used variable (authored by dblaikie).
Use default ref capture to avoid unused capture warning on assert-used variable
Thu, Apr 8, 5:50 PM
dblaikie added a comment to D100137: [JumpThreading] merge debug info when merging select+br.

The syntax the LLVM project uses to reference bugs is "PR<number>" (eg: PR39531) not "pr/<number>", if you could update the patch description to match that format. (this is also supported on llvm.org, where you can go to llvm.org/PR39531 to reach that bug, for instance)

Thu, Apr 8, 5:18 PM · debug-info, Restricted Project
dblaikie added a comment to D99900: [llvm] [testsuite] Fix invalid DW_AT_location DWARF expression in unattached-global.ll.

Yeah, my understanding is that all global variables start out as both !dbg attachements on LLVM globals *and* in the list of globals in the DICompileUnit. This way we don't loose track of them when the LLVM global symbol gets deleted by an optimization.

Thu, Apr 8, 2:21 PM · Restricted Project
dblaikie added a project to D100137: [JumpThreading] merge debug info when merging select+br: debug-info.
Thu, Apr 8, 2:06 PM · debug-info, Restricted Project
dblaikie added a comment to D100137: [JumpThreading] merge debug info when merging select+br.

Link: b/184085493
Link: https://bugs.llvm.org/show_bug.cgi?id=39531
Link: https://www.llvm.org/docs/HowToUpdateDebugInfo.html#when-to-merge-instruction-locations
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>

Thu, Apr 8, 2:06 PM · debug-info, Restricted Project
dblaikie added a comment to D99751: Add the TableGen assert statement, step 3.

Ah, so I take it from what you're saying that I can start a new branch and then do a revert of an arbitrary commit on it? I guess there is no reason why not.

Thu, Apr 8, 11:14 AM · Restricted Project
dblaikie added a comment to D99751: Add the TableGen assert statement, step 3.

The reason I haven't reverted this patch is because I don't feel comfortable doing a revert with the tangled up branches I have. Do you think it's safe to revert the commit on my 'assert' branch even though it is not the topmost commit?

Thu, Apr 8, 10:48 AM · Restricted Project
dblaikie added a comment to D99751: Add the TableGen assert statement, step 3.

I just pushed this revision and I have no idea how. I meant to push the list slice suffix revision, which I did, and somehow this one went along with it.

Thu, Apr 8, 10:36 AM · Restricted Project
dblaikie added a comment to D99900: [llvm] [testsuite] Fix invalid DW_AT_location DWARF expression in unattached-global.ll.

Maybe I'm missing something but it looks to me like this test is supposed to test that we don't attach DW_AT_location to a variable that has been optimized out.

This test is what you get after optimizing away

someglobal = i32 ..., !dbg !3

and I don't think it is invalid at all. Since it doesn't hold a constant, the DIExpression is meaningless without being attached to a global, but it's not invalid IR. It just shouldn't get output in DWARF.

This going to get less ambiguous with the ongoing work to make arguments in DIExpressions more explicit.

Thu, Apr 8, 10:35 AM · Restricted Project

Wed, Apr 7

dblaikie added inline comments to D99698: [DWARF] Fix crash for DWARFDie::dump..
Wed, Apr 7, 7:57 PM · Restricted Project
dblaikie accepted D99933: [Debug-Info] Use inlined strings in .dwinfo section by default for DBX..

Looks good - thanks!

Wed, Apr 7, 7:51 PM · Restricted Project
dblaikie added a comment to D100068: [llvm-dwarfdump] Fix slowdown in dwarfdump statistics..

If there are cases where this causes the wrong/different answer (@rdhindsa you mentioned in some targets with this patch the statistics changed?) perhaps this patch could include a test demonstrating that bug/fix?

Wed, Apr 7, 2:37 PM · Restricted Project
dblaikie added a comment to D99900: [llvm] [testsuite] Fix invalid DW_AT_location DWARF expression in unattached-global.ll.

@aprantl @JDevlieghere do you folks want to update the verifier in any way to make this sort of thing invalid?

Before implementing it into DWARFExpression::verify I see two issues:

  • IIUC the verifier would be run by llvm-dwarfdump --verify which is currently not being run.
  • The goal of this testcase is to omit DW_AT_location when producing the binary. Therefore the invalid DWARF expression would not be written out at all. So it could not be verified as incorrect by llvm-dwarfdump.
Wed, Apr 7, 1:45 PM · Restricted Project
dblaikie added a comment to D99709: [DebugInfo] Avoid generating DW_TAG_GNU_call_site if no DW_TAG_GNU_call_site_parameter present.

I think some LLDB tests would fail for sure...

Is there LLVM test coverage for this? If not it'd be good to make sure some is added.

There are (the premerge also shows it). By applying this patch we have:

Failed Tests:
LLVM :: CodeGen/X86/xray-custom-log.ll
LLVM :: DebugInfo/AArch64/unretained-declaration-subprogram.ll
LLVM :: DebugInfo/MIR/ARM/call-site-info-vmovd.mir
LLVM :: DebugInfo/MIR/ARM/call-site-info-vmovs.mir
LLVM :: DebugInfo/MIR/Hexagon/bundled-call-pr44001.mir
LLVM :: DebugInfo/MIR/X86/call-site-gnu-vs-dwarf5-attrs.mir
LLVM :: DebugInfo/MIR/X86/debug-call-site-param.mir
LLVM :: DebugInfo/X86/convert-loclist.ll
LLVM :: DebugInfo/X86/dbg-call-site-undef-params.ll
LLVM :: DebugInfo/X86/debug_addr.ll
LLVM :: DebugInfo/X86/dwarf-callsite-related-attrs.ll
LLVM :: DebugInfo/X86/fission-call-site.ll
LLVM :: DebugInfo/X86/lto-cross-cu-call-origin-ref.ll
LLVM :: DebugInfo/X86/ranges_always.ll
LLVM :: tools/llvm-dwarfdump/X86/stats-dbg-callsite-info.ll
Wed, Apr 7, 12:28 PM · Restricted Project, debug-info
dblaikie added inline comments to D99933: [Debug-Info] Use inlined strings in .dwinfo section by default for DBX..
Wed, Apr 7, 12:27 PM · Restricted Project

Tue, Apr 6

dblaikie added a comment to D95617: [DWARF] Inlined variables with no location should not have a DW_TAG_variable.

Oh, interesting note I just realized in some other code I was investigating (for https://bugs.llvm.org/show_bug.cgi?id=49769 ) - this sort of change should be made for non-inline functions too. WEll, specifically non-inline functions that have an abstract origin.

Tue, Apr 6, 6:21 PM · Restricted Project
dblaikie added a comment to D99561: Support visitor pattern by PointerUnion..

also the issue of "Oh, I'm used to using x.match(a, b) - ah, bother, this is the standard thing that doesn't support x.match, I have to use visit(a, b, x) instead".

The same thing could be said about many of our APIs in Support/ADT which we make in a form that we think is more convenient than the standard, like how llvm::sort and all the others we have don't need .begin()/.end() to operate on the entire range. I'm annoyed every time I have to an std:: entry point that forces me in the extra boilerplate, but I wouldn't want to remove these nicer APIs from LLVM for "consistency with the standard".
(don't forget also that the standard often will have constraint around API design we don't have, for example around exception handling).

Tue, Apr 6, 5:41 PM · Restricted Project
dblaikie updated subscribers of D99698: [DWARF] Fix crash for DWARFDie::dump..

Test case?

I am not sure how to test it.

How about a unit test? We have some for the DWARF parser already.

Ah missed it.
Are those YAML tests manually constructed? I tried obj2yaml

Tue, Apr 6, 5:38 PM · Restricted Project
dblaikie added inline comments to D99883: [TableGen] Make behavior of list slice suffix consistent across all values.
Tue, Apr 6, 5:25 PM · Restricted Project
dblaikie updated subscribers of D98477: [ADT] Add IntrusiveVariant.
Tue, Apr 6, 4:47 PM · Restricted Project
dblaikie added inline comments to D98799: [UniqueLinkageName] Use consistent checks when mangling symbo linkage name and debug linkage name..
Tue, Apr 6, 4:10 PM · Restricted Project
dblaikie updated subscribers of D99900: [llvm] [testsuite] Fix invalid DW_AT_location DWARF expression in unattached-global.ll.

@aprantl @JDevlieghere do you folks want to update the verifier in any way to make this sort of thing invalid? (looks like it might be relatively easy to test for, judging by the code touched in D99850)

Tue, Apr 6, 4:08 PM · Restricted Project
dblaikie added inline comments to D99883: [TableGen] Make behavior of list slice suffix consistent across all values.
Tue, Apr 6, 4:06 PM · Restricted Project
dblaikie added a comment to D99179: [RFC] [Coroutines] Enable printing coroutine frame in debugger if program is compiled with -g.

(fundamental design discussion/decision might be worth a broader design discussion on llvm-dev with a bunch of debug info folks - not sure)

Tue, Apr 6, 4:02 PM · debug-info, Restricted Project
dblaikie added a comment to D99179: [RFC] [Coroutines] Enable printing coroutine frame in debugger if program is compiled with -g.

Testing looks a bit better - could probably be more comprehensive.

Tue, Apr 6, 4:01 PM · debug-info, Restricted Project
dblaikie added a comment to D99561: Support visitor pattern by PointerUnion..

To chime in on the spelling of things relative to std, I had similar questions for IntrusiveVariant in https://reviews.llvm.org/D98477

A natural question that might help with deciding how to handle visit is: how should we spell alternative type access? std uses the free function std::get. It makes sense, as that same free function is overloaded for e.g. std::tuple, std::pair, std::variant, etc. but we don't currently have anything similar in ADT.

I think my leaning would be to go all-in on the std approach by doing the following:

  • Add an llvm::adt namespace (or something similar) containing things which are "designed to fulfill the same roles as the identically-named member of the std namespace, but for whatever reason we cannot actually use that member directly". Generally the reason will be: that thing is not an extension point, and it is forbidden to re-open std to overload that thing.
Tue, Apr 6, 3:56 PM · Restricted Project
dblaikie added a comment to D99825: [cmake] Enable -Werror=return-type for clang.

The two I see are date-time and unguarded-availability-new

There's also non-virtual-dtor and suggest-override.

Tue, Apr 6, 3:43 PM · Restricted Project
dblaikie added a comment to D99825: [cmake] Enable -Werror=return-type for clang.
  1. I've updated the summary to include wording from the original diff D98224 which points out that -Werror=return-type is used in libcxx, libcxxabi, and libunwind.
Tue, Apr 6, 1:02 PM · Restricted Project
dblaikie requested changes to D99825: [cmake] Enable -Werror=return-type for clang.

I don't think this is appropriate/the right direction - we have a general mechanism for turning on warnings-as-errors for a build that I'd expect people to use if they want that behavior. I don't think we want to start opting in specific warnings to warnings-as-error status overriding the users request via the general flag.

Tue, Apr 6, 10:47 AM · Restricted Project
dblaikie updated subscribers of D99650: Demonstrating lack of thread-safety in BackendUtil.

I think there's a way to post a Phab 'reivew' that won't send mail to the -commits lists, but I forget what it is. I thought it might've been some metadata tag, but can't find it (maybe @MaskRay or @mehdi_amini recall what the magic was)

Tue, Apr 6, 10:44 AM · Restricted Project
dblaikie added a comment to D99160: [X86][FastISEL] Support DW_TAG_call_site_parameter with FastISEL.

I think that the Debug Entry Values feature should not be enabled by default for non optimized code, so the TargetOptions::ShouldEmitDebugEntryValues() should be patched with checking of optimization level (it should be > 0).

That's currently intended to be already handled by the frontend, right? (clang only sets EnableDebugEntryValues (which ShouldEmitDebugEntryValues checks (hmm, it checks under 'or', not 'and', so I'm not sure where the "only above -O0" is implemented, but it is implemented somewhere?) if optimizations are enabled, yeah?)

Oh, is entry_values actually not conditionalized? It's only the call_site support that's currently conditionalized on "above -O0"?

Looks like there is no explicit check of optimization level (above "-O0"), neither on frontend nor backend for entry-values generation. I think it is the situation since there should not be any optimization (at least that I am aware of, in the case of C/C++) that would cause the entry-values generation...

Hmm - If that's the case, and we currently have some cases where entry_values are emitted at -O0, I'm not sure /not/ emitting those is the right call either. If we believe/have data to show that there are so few useful uses of entry_value at -O0 that it's not worth the DWARF size growth to put call_sites in at -O0, then I think it might still be worth leaving the entry_values in (unless they take up a bunch of extra space) for the cases of mixed optimization compilation (-O0 some code you're debugging, but building the rest with optimizations).

Yeah... That is valuable example... I am thinking in that direction as well, and I am closer to the enabling it for -O0 case if that is useful (and there is no dramatic cost in terms of DWARF size).

Tue, Apr 6, 10:43 AM · Restricted Project, Restricted Project, debug-info
dblaikie added a comment to D99709: [DebugInfo] Avoid generating DW_TAG_GNU_call_site if no DW_TAG_GNU_call_site_parameter present.

I think some LLDB tests would fail for sure...

Tue, Apr 6, 10:11 AM · Restricted Project, debug-info
dblaikie added inline comments to D99933: [Debug-Info] Use inlined strings in .dwinfo section by default for DBX..
Tue, Apr 6, 10:10 AM · Restricted Project

Mon, Apr 5

dblaikie added a comment to D99698: [DWARF] Fix crash for DWARFDie::dump..

Test case?

I am not sure how to test it.

Mon, Apr 5, 2:12 PM · Restricted Project
dblaikie added a comment to D99160: [X86][FastISEL] Support DW_TAG_call_site_parameter with FastISEL.

I think that the Debug Entry Values feature should not be enabled by default for non optimized code, so the TargetOptions::ShouldEmitDebugEntryValues() should be patched with checking of optimization level (it should be > 0).

Mon, Apr 5, 2:11 PM · Restricted Project, Restricted Project, debug-info
dblaikie added inline comments to D99883: [TableGen] Make behavior of list slice suffix consistent across all values.
Mon, Apr 5, 2:04 PM · Restricted Project

Sun, Apr 4

dblaikie accepted D98994: NFC. Refactored DIPrinter for better support of new print styles..

Looks good to me - one optional rename. I don't have a great idea of an alternative though.

Sun, Apr 4, 7:12 PM · Restricted Project
dblaikie added a comment to D99850: [WIP/RFC] lld LTO drops variables in namespaces from .debug_names.

Yeah, that looks like it breaks the unattached-global.ll because this causes a DW_AT_location to be added (probably with some kind of broken/empty expression) for a variable without a location/that's been optimized away.

I did not expect that but IMO unattached-global.ll is wrong (since its inception in D20147):

!4 = !DIExpression(DW_OP_plus_uconst, 4)
                DW_AT_location	(DW_OP_plus_uconst 0x4)

That is not a valid DWARF expression for DW_TAG_variable.

Sun, Apr 4, 6:28 PM · Restricted Project
dblaikie added a comment to D99560: Utility to construct visitors from lambdas..

It’s impossible to statically dispatch to the correct lambda with such API (or at least a very difficult exercise). std::visit takes a single visitor and multiple variants, not the other way around. Stitching lambdas together is just one way of constructing a visitor type, it can be a plain old class with overloaded operator() set.

Sun, Apr 4, 8:34 AM · Restricted Project

Sat, Apr 3

dblaikie added inline comments to D98994: NFC. Refactored DIPrinter for better support of new print styles..
Sat, Apr 3, 7:21 PM · Restricted Project
dblaikie added a comment to D99850: [WIP/RFC] lld LTO drops variables in namespaces from .debug_names.

Yeah, that looks like it breaks the unattached-global.ll because this causes a DW_AT_location to be added (probably with some kind of broken/empty expression) for a variable without a location/that's been optimized away.

Sat, Apr 3, 6:33 PM · Restricted Project
dblaikie committed rG30df6d5d6a85: Preprocessor conditionalize some assert-only functions to suppress -Wunused… (authored by dblaikie).
Preprocessor conditionalize some assert-only functions to suppress -Wunused…
Sat, Apr 3, 2:21 PM
dblaikie committed rG9f6649dd1249: Add void cast to suppress -Wunused-member-variable on assert-only member (authored by dblaikie).
Add void cast to suppress -Wunused-member-variable on assert-only member
Sat, Apr 3, 2:21 PM
dblaikie committed rG499571ea835d: Add workaround for false positive in -Wfree-nonheap-object (authored by dblaikie).
Add workaround for false positive in -Wfree-nonheap-object
Sat, Apr 3, 2:21 PM
dblaikie committed rG2554f99b554f: Opaque pointers: Migrate examples to use load with explicit type (authored by dblaikie).
Opaque pointers: Migrate examples to use load with explicit type
Sat, Apr 3, 2:21 PM

Fri, Apr 2

dblaikie added a comment to D99560: Utility to construct visitors from lambdas..

std::forward with inheritance does not remove a need to invoke a copy constructors, I managed to implement overloaded using a struct member and super ugly SFINAE that does properly captures lambdas by reference when lambda is a lvalue, however it still does need to copy when lambda is rvalue: https://godbolt.org/z/dqshPW6h1 (top most example, I just count the number of HeavyStuff::HeavyStuff(HeavyStuff const&) copy constructor calls in the generated code). But it does look much worse than inheritance.

Though with -O3 version with forward and version with struct member generate less code than copying by value.

Fri, Apr 2, 11:49 PM · Restricted Project
dblaikie committed rG2458aa0b9136: Add missing override to clang tblgen AttrEmitter (authored by dblaikie).
Add missing override to clang tblgen AttrEmitter
Fri, Apr 2, 9:00 PM
dblaikie added inline comments to D98994: NFC. Refactored DIPrinter for better support of new print styles..
Fri, Apr 2, 3:48 PM · Restricted Project
dblaikie added a comment to D98477: [ADT] Add IntrusiveVariant.

Perhaps PointerUnion could be implemented as a derivation from this intrusive variant in a follow-up change, subsuming D99561

I'm not as familiar with the implementation of PointerUnion, I just noted when I did a pass over the LLVM container types that it wouldn't be sufficient for the specific thing I was working on, so I started writing my own compact variant-like type. At the very least I think we should have a common llvm::visit with specializations for both llvm::PointerUnion and llvm::IntrusiveVariant.

Fri, Apr 2, 1:25 PM · Restricted Project
dblaikie added a comment to D99560: Utility to construct visitors from lambdas..

Not asking you to do anything necessarily - though I think it's worth someone (you or @lhames perhaps) checking if the abstraction generalizes well to other/existing use cases.

This is dispatching based on static type, handleAllErrors is dispatching based on the dynamic type of the error. I suspect that those two use-cases are different enough that there's limited value in unifying the code.

Fri, Apr 2, 1:12 PM · Restricted Project
dblaikie added inline comments to D99179: [RFC] [Coroutines] Enable printing coroutine frame in debugger if program is compiled with -g.
Fri, Apr 2, 12:26 PM · debug-info, Restricted Project
dblaikie added inline comments to D78778: [AMDGPU] Add SupportsDebugOnlyCFI to MCAsmInfo.
Fri, Apr 2, 12:16 PM · debug-info, Restricted Project
dblaikie accepted D99731: [nfc] [llvm] Make DWARFListTableBase::findList const.

If it compiles (looks like it should - don't see any caching in member variables, etc), seems fine by me.

Fri, Apr 2, 11:57 AM · Restricted Project
dblaikie added a comment to D99334: [TransformUtils] Don't generate invalid llvm.dbg.cu .

what was invalid about the metadata? The llvm.dbg.cu list was empty? I wonder if we should just say that's valid. It seems pretty benign/reasonable - makes it easier to write code like this, or code that, say, stripped a CU out wouldn't have to worry about removing a potentially zero-length cu list. @aprantl @JDevlieghere ?

I don't think that's a good idea. It complexifies every piece of user code around handling "llvm.dbg.cu" if the user can't assume it actually contains what it's supposed to. Doing so is a tradeoff of one if statement in CloneFunctionInto becoming an extra if statement around every use of "llvm.dbg.cu". That's not a good tradeoff in terms of reliability or ergonomics.

Fri, Apr 2, 11:56 AM · Restricted Project
dblaikie added a comment to D99560: Utility to construct visitors from lambdas..

Regarding deriving from using other kinds of functors: I have not yet seen a need for this. My preference is to cross that bridge when we reach it. :)

Regarding taking references to the functors: I played around but on my first attempts the assembly seemed to have become scarier than without the references, so I decided not to push it further. Here's the evidence: https://godbolt.org/z/eoGKMW3jx

Fri, Apr 2, 11:34 AM · Restricted Project
dblaikie added a comment to D99561: Support visitor pattern by PointerUnion..

Aligned the calling convention of visit to match that of std::visit. Renamed the more convenient version of visit to match to have the best of the two worlds: better ergonomics and not clashing with std::visit.

Fri, Apr 2, 11:30 AM · Restricted Project
dblaikie added inline comments to D99238: [DebugInfo] Enable the call site parameter feature by default.
Fri, Apr 2, 11:12 AM · Restricted Project, Restricted Project, debug-info
dblaikie added inline comments to D99238: [DebugInfo] Enable the call site parameter feature by default.
Fri, Apr 2, 10:44 AM · Restricted Project, Restricted Project, debug-info

Thu, Apr 1

dblaikie added a comment to D99698: [DWARF] Fix crash for DWARFDie::dump..

Test case?

Thu, Apr 1, 5:29 PM · Restricted Project
dblaikie added inline comments to D98289: [lldb] 2/2: Fix DW_AT_ranges DW_FORM_sec_offset not using DW_AT_rnglists_base (used by GCC).
Thu, Apr 1, 5:28 PM · Restricted Project
dblaikie accepted D99703: [debug-info][XCOFF] set `-gno-column-info` by default for DBX.

Sounds good to me

Thu, Apr 1, 12:52 PM · Restricted Project, debug-info
dblaikie added a comment to D99702: [lldb-vscode] Use LLVM's ScopeExit to ensure we always terminate the debugger.

@wallace - minor inconvenience, but approving a patch in Phabricator without any textual comment does not result in an email to the list, making it look like a patch is being committed without approval - could you include some text in your Phab approvals, as it makes it a bit easier when doing post-commit/process review?

Thu, Apr 1, 12:26 PM · Restricted Project

Wed, Mar 31

dblaikie added a comment to D99561: Support visitor pattern by PointerUnion..

True llvm::visit API similar to std::visit requires visited type to be a std::variant (llvm::variant?) and implementng it is a lot of code (e.g. implementation in absl). Given that it's unlikely that PointerUnion at any point will become std::variant, I'd say that it will always be a separate API. And for this usability improvement the change is not that large.

Wed, Mar 31, 2:46 PM · Restricted Project

Tue, Mar 30

dblaikie added a comment to D98477: [ADT] Add IntrusiveVariant.

Perhaps PointerUnion could be implemented as a derivation from this intrusive variant in a follow-up change, subsuming D99561

Tue, Mar 30, 9:16 PM · Restricted Project
dblaikie added a comment to D98477: [ADT] Add IntrusiveVariant.

Looks like this visitor infrastructure has some overlap with D99560 and D99561 - be good to deduplicate some work here, perhaps.

Tue, Mar 30, 9:14 PM · Restricted Project
dblaikie updated subscribers of D99561: Support visitor pattern by PointerUnion..

Thanks for taking a look, Mehdi and David! PTAL.

David, it was a goal to try to keep the API similar to std::variant. The biggest difference is that std::visit is a static function that accepts variable number of variants. I never experienced a need for that and replaced it with a non-static method (less verbose, more idiomatic, less complex). In my mind this API difference is insignificant, and the APIs are similar enough, but maybe when you say similar you mean something different?

Tue, Mar 30, 9:11 PM · Restricted Project
dblaikie added a comment to D99560: Utility to construct visitors from lambdas..

Re: handleAllErrors. Maybe. It might result in better runtime performance (because there is not recursion on the handlers), but it's also a rewrite. I am not sure if you are asking me to do something. :)

Tue, Mar 30, 8:59 PM · Restricted Project
dblaikie added a comment to D99055: [llvm-objcopy] Refactor CopyConfig structure..

So the general point of this change is that previously all the options were in one struct which meant that, say, the MachO implementation could accidentally end up using an ELF option value, etc? And this way the implementations can accept the common options (the base class) and their implementation-specific options?

Tue, Mar 30, 8:47 PM · Restricted Project
dblaikie accepted D99626: [llvm-objcopy] Unify format specific options check..

Seems OK /if/ we go in the direction of D99055 (so please hold off on committing this until that one's approved/settled). Specifically: If we don't go in that direction, it seems good that each of these checks are in the format-specific parts of the code, rather than all together in the main part of the code.

Tue, Mar 30, 8:35 PM · Restricted Project
dblaikie updated subscribers of D99334: [TransformUtils] Don't generate invalid llvm.dbg.cu .

what was invalid about the metadata? The llvm.dbg.cu list was empty? I wonder if we should just say that's valid. It seems pretty benign/reasonable - makes it easier to write code like this, or code that, say, stripped a CU out wouldn't have to worry about removing a potentially zero-length cu list. @aprantl @JDevlieghere ?

Tue, Mar 30, 3:26 PM · Restricted Project
dblaikie committed rGae217bf1f327: Conditionalize the JIT test dependency (authored by dblaikie).
Conditionalize the JIT test dependency
Tue, Mar 30, 1:04 PM
dblaikie added a comment to D99534: [dsymutil] Relocate DW_TAG_label.

Any uses of low_pc other than the CU's low_pc that you know of that shouldn't be handled in this way? Maybe the positive list (inlined, lexical, label) should be removed?

Tue, Mar 30, 12:56 PM · Restricted Project
dblaikie added a comment to D99561: Support visitor pattern by PointerUnion..

Needs some unit tests.

Tue, Mar 30, 11:40 AM · Restricted Project
dblaikie updated subscribers of D99560: Utility to construct visitors from lambdas..

Needs some unit tests.

Tue, Mar 30, 10:58 AM · Restricted Project
dblaikie updated subscribers of D99580: [CLANG] [DebugInfo] Convert File name to native format.

@rnk @akhuang - I think we touched on this before maybe in some other thread/patch/discussion?

Tue, Mar 30, 10:52 AM · Restricted Project, Restricted Project

Mon, Mar 29

dblaikie committed rGbd56e91fdbc6: Add missing dependency to fix building the jit tests (authored by dblaikie).
Add missing dependency to fix building the jit tests
Mon, Mar 29, 5:33 PM
dblaikie added a comment to D99444: [ADT] Introduce lazy unique iterator.

I probably wouldn't mind an algorithm-like function, rather than an iterator (not composable, but avoids some of the harder design issues) - llvm::for_each_unique_element(R, [](const T& t) { ... });

Mon, Mar 29, 9:55 AM · Restricted Project
dblaikie added a comment to D99444: [ADT] Introduce lazy unique iterator.

Yeah, not entirely sure this abstraction carries its weight. (I was going to suggest the set could be moved from the iterator into a range object (rather than using iterator_range) but then copies of the iterator wouldn't be independent (only one of them would visit a given element and the other would skip it) & then that starts to make me worry about the abstraction in general - making the iterators a bit big/expensive to copy/etc)

Mon, Mar 29, 9:54 AM · Restricted Project
dblaikie added a comment to D98146: OpaquePtr: Turn inalloca into a type attribute.
Mon, Mar 29, 9:46 AM · Restricted Project
dblaikie updated subscribers of D99055: [llvm-objcopy] Refactor CopyConfig structure..

Thanks for the update. I do have some concerns about the multiple inheritance stuff going on, if I'm honest, as usually this is a design smell

Mon, Mar 29, 8:26 AM · Restricted Project

Fri, Mar 26

dblaikie added inline comments to D99401: Fix .debug_aranges parsing issues introduced with llvm error handling in LLDB.
Fri, Mar 26, 3:07 PM · Restricted Project
dblaikie added a comment to D99401: Fix .debug_aranges parsing issues introduced with llvm error handling in LLDB.

(generally makes sense to me - but I'll leave it to some more lldb-focussed reviewers to do more review/approval)

Fri, Mar 26, 10:24 AM · Restricted Project

Thu, Mar 25

dblaikie added a comment to D99250: [DebugInfo] Fix the mismatching of C++ language tags and Dwarf versions..

But if folks supporting previous builds of DBX are likely going to need a bunch of other compatibility work - then maybe adding the debugger tuning may be suitable/necessary/worthwhile at this point.

Yes, there are a few of the feature flags setting for compatibility coming.. Thanks David!

Thu, Mar 25, 11:56 AM · debug-info, Restricted Project
dblaikie added a comment to D99250: [DebugInfo] Fix the mismatching of C++ language tags and Dwarf versions..

But if folks supporting previous builds of DBX are likely going to need a bunch of other compatibility work - then maybe adding the debugger tuning may be suitable/necessary/worthwhile at this point.

Thu, Mar 25, 11:28 AM · debug-info, Restricted Project
dblaikie added a comment to D99250: [DebugInfo] Fix the mismatching of C++ language tags and Dwarf versions..

Does anyone else have a DWARFv3 consumer they care about? (@aprantl and @probinson)
Does anyone have a DWARF consumer that changes significantly based on the language version (I guess lldb (@aprantl can you confirm?)? I doubt gdb does (yeah, it just treats all C++ versions the same)? how about Sony?)

Thu, Mar 25, 11:28 AM · debug-info, Restricted Project