Page MenuHomePhabricator

dblaikie (David Blaikie)
User

Projects

User Details

User Since
Oct 8 2012, 9:19 AM (546 w, 5 d)

Recent Activity

Wed, Mar 29

dblaikie committed rGa73d354024f6: DWARF: Enable "ranges always" under Split DWARF by default (authored by dblaikie).
DWARF: Enable "ranges always" under Split DWARF by default
Wed, Mar 29, 2:02 PM · Restricted Project, Restricted Project
dblaikie committed rG40803282b788: DwarfDebug: Minor condition cleanups (authored by dblaikie).
DwarfDebug: Minor condition cleanups
Wed, Mar 29, 2:02 PM · Restricted Project, Restricted Project

Tue, Mar 28

dblaikie added inline comments to D146662: DebugInfo: Rebuild dwp debug_info index column from v5 indexes more robustly.
Tue, Mar 28, 1:47 PM · Restricted Project, Restricted Project
dblaikie committed rG22afe19ac03f: DebugInfo: Rebuild dwp debug_info index column from v5 indexes more robustly (authored by dblaikie).
DebugInfo: Rebuild dwp debug_info index column from v5 indexes more robustly
Tue, Mar 28, 1:47 PM · Restricted Project, Restricted Project
dblaikie closed D146662: DebugInfo: Rebuild dwp debug_info index column from v5 indexes more robustly.
Tue, Mar 28, 1:47 PM · Restricted Project, Restricted Project
dblaikie committed rG302079ae9ac0: Simplify index rebuilding test. (authored by dblaikie).
Simplify index rebuilding test.
Tue, Mar 28, 1:25 PM · Restricted Project, Restricted Project
dblaikie added a comment to D146854: [Symbolizer] Add flag to dump symbolizer markup context JSON..

"context" as the name for this confuses me a bit - perhaps you can describe it more (maybe I'm jus tthinking about it wrong, or maybe it could benefit from a rename)

Tue, Mar 28, 8:35 AM · Restricted Project, Restricted Project

Mon, Mar 27

dblaikie added a comment to D138247: PR58819: Correct linkage and mangling of lambdas in inline static member initializers.

The relevant text of the current Itanium ABI (which was updated in https://github.com/itanium-cxx-abi/cxx-abi/commit/d8e9d102c83f177970f0db6cc8bee170f2779bc1)

In the following contexts, however, the one-definition rule requires closure types in different translation units to "correspond":

  • default arguments appearing in class definitions
  • default member initializers
  • the bodies of inline or templated functions
  • the initializers of inline or templated variables

Could you update the references to the ABI document to use the new text?

Mon, Mar 27, 5:01 PM · Restricted Project, Restricted Project
dblaikie updated the diff for D138247: PR58819: Correct linkage and mangling of lambdas in inline static member initializers.

Rename VariableTemplate classification to TemplatedVariable to match ABI wording

Mon, Mar 27, 5:01 PM · Restricted Project, Restricted Project
dblaikie updated the diff for D138247: PR58819: Correct linkage and mangling of lambdas in inline static member initializers.

Remove StaticDataMember and classify static member variables of templates as variable templates

Mon, Mar 27, 4:52 PM · Restricted Project, Restricted Project
dblaikie added inline comments to D145803: [clang][DebugInfo] Emit DW_AT_type of preferred name if available.
Mon, Mar 27, 4:30 PM · Restricted Project, Restricted Project, Restricted Project
dblaikie added a comment to D146986: Downgrade reserved module identifier error into a warning.

From the discussion on the issue:

Do we want this loosening of the restriction to apply to *only* std and the same followed by a number, or to any reserved identifier used in a module? e.g.,

module std; // error today, but will become a warning
module _Test; // error today, but do we want this to become a warning as well?

my thinking is we probably want all of these to be warnings because it'd be hard to explain why std is reserved but with a warning while _Test is reserved but with an error.

Yeah, I'd treat them equally - while we could subset the reserved names and allow implementations to only use a subset (while leaving the rest as an error for both implementations and consumers alike) that doesn't feel in keeping with the purpose of these names - to be usable by /someone/ and so necessary to allow them to be used.

(hmm - there's some discussion in the description about the fact that this error was already suppressed in "system headers" - why was that suppression inadequate for system implementation modules? (& does that suppression for reserved names risk being over-broad, since every third party library installed on a system is generally considered a "system header", even if they aren't part of the implementation?))

We currently use line markers to "enter" a system header and that's quite fragile. I mentioned we could use #pragma clang system_header, but @ChuanqiXu didn't think that was appropriate because these are not headers, they're modules, and we should have some separation between "system headers" and "system modules". (https://github.com/llvm/llvm-project/issues/61446#issuecomment-1473029776) As for being over-broad, it might be, but this is the approach we usually take (anything that's a "system header" is considered special and gets less diagnostics because the user isn't typically able to change the contents of the header file anyway).

Mon, Mar 27, 12:28 PM · Restricted Project, Restricted Project
dblaikie added a comment to D146595: [clang] Add "transparent_stepping" attribute.

I know this is a bit of a redirection/scope creep/etc - but I'd quite like to see a solution that is likely to be usable for the "std::function" problem (stepping into std::function should allow you to reach the underlying function - but lldb currently skips any call to a std-namespaced function, I think, so you step right over the whole op() call) that could also cover the Swift needs. Though perhaps they're just sufficiently different problems that there is no generalizing here.

This patch aims at exposing an existing LLVM IR & DWARF feature in clang.

Mon, Mar 27, 11:57 AM · debug-info, Restricted Project, Restricted Project, Restricted Project
dblaikie added inline comments to D144565: dwp check overflow.
Mon, Mar 27, 11:40 AM · Restricted Project, Restricted Project
dblaikie added inline comments to D145803: [clang][DebugInfo] Emit DW_AT_type of preferred name if available.
Mon, Mar 27, 11:38 AM · Restricted Project, Restricted Project, Restricted Project
dblaikie added a comment to D146986: Downgrade reserved module identifier error into a warning.

From the discussion on the issue:

Do we want this loosening of the restriction to apply to *only* std and the same followed by a number, or to any reserved identifier used in a module? e.g.,

module std; // error today, but will become a warning
module _Test; // error today, but do we want this to become a warning as well?

my thinking is we probably want all of these to be warnings because it'd be hard to explain why std is reserved but with a warning while _Test is reserved but with an error.

Mon, Mar 27, 11:37 AM · Restricted Project, Restricted Project
dblaikie accepted D146852: [DWARF][GDB INDEX] Fix to deal with constant pool de-dupliation Summary:.

Thanks!

Mon, Mar 27, 11:12 AM · Restricted Project, Restricted Project
dblaikie added a comment to D146595: [clang] Add "transparent_stepping" attribute.

I know this is a bit of a redirection/scope creep/etc - but I'd quite like to see a solution that is likely to be usable for the "std::function" problem (stepping into std::function should allow you to reach the underlying function - but lldb currently skips any call to a std-namespaced function, I think, so you step right over the whole op() call) that could also cover the Swift needs. Though perhaps they're just sufficiently different problems that there is no generalizing here.

Mon, Mar 27, 11:09 AM · debug-info, Restricted Project, Restricted Project, Restricted Project
dblaikie added inline comments to D146899: [Docs] Added llvm-reduce docs in commandGuide.
Mon, Mar 27, 10:46 AM · Restricted Project, Restricted Project
dblaikie updated subscribers of D146899: [Docs] Added llvm-reduce docs in commandGuide.
Mon, Mar 27, 10:33 AM · Restricted Project, Restricted Project
dblaikie accepted D146892: [documentation] Fix some typos.

Looks good, thanks!

Mon, Mar 27, 10:31 AM · Restricted Project, Restricted Project, Restricted Project

Sat, Mar 25

dblaikie added a comment to D146852: [DWARF][GDB INDEX] Fix to deal with constant pool de-dupliation Summary:.

Hmm, the way we're reading this doesn't seem like it /quite/ makes sense - we read the offsets that are encoded in the "symbol table" but then we read the constant pool CU vectors directly based on the number of unique offsets in the symbol table.

Sat, Mar 25, 12:06 PM · Restricted Project, Restricted Project

Thu, Mar 23

dblaikie added a comment to D146595: [clang] Add "transparent_stepping" attribute.

What would happen if, instead, these trampolining functions were annotated nodebug? I guess then you wouldn't have the top level one as an entry point for the user, as there would be no debug info for it?

These trampoline functions can call other functions as part of their setup, so annotating them with nodebug wouldn't guarantee that the debugger would be able to step through to the correct place. Additionally, you wouldn't be able to refer to them in LLDB expression evaluation either.

Oh, that's a bit worrying - that these aren't trampolines in the sort of lower-level sense, only in a high level sense.

Is the ability to debug these functions under some debugger flag ("show me intermediate steps/implementation details") a priority? Would it be OK if that were only available with a rebuild (ie: maybe didn't need to be represented in DWARF - this would also allow debug info to be smaller & not having to describe these transparent/implementation detail functions)?

You mean step in the trampoline? It'd be nice, but not absolutely required. The main issue would be not being able to evaluate expressions though.

Thu, Mar 23, 5:40 PM · debug-info, Restricted Project, Restricted Project, Restricted Project
dblaikie added a comment to D146466: [clang] diagnose function fallthrough.

I think the reason "recoverable" ubsan causes trouble is that it introduces branches that subsequent optimizations can abuse. So without ubsan, we just have an udiv instruction. With ubsan, we conveniently have a branch on exactly the condition that would make the udiv undefined, so we can easily prove control flow doesn't continue after the ubsan handler. Subsequent optimizations take advantage of that, so ubsan "breaks" code. (So the code was never actually correct according to the semantic model, but it was broken in a way the compiler is less likely optimize.)

Thu, Mar 23, 5:02 PM · Restricted Project, Restricted Project, Restricted Project
dblaikie added a comment to D146595: [clang] Add "transparent_stepping" attribute.

Should there be some check that that symbol exists? (the current test uses the unmangled name "bar" for instance - which gives the wrong impression?)

You mean as part of the attribute validation?

Thu, Mar 23, 3:38 PM · debug-info, Restricted Project, Restricted Project, Restricted Project
dblaikie added a comment to D146466: [clang] diagnose function fallthrough.

(recoverable feels like a bit of a distraction here? recoverable just means you've asked ubsan not to trap/stop on failure - but to let the program continue and do whatever it would've done without the sanitizer enabled - sometimes that's crash/trap anyway, sometimes it's something less bad... but that's all that's being asked for: "keep going/do whatever you'd do without the sanitizer enabled, rather than hard stop as soon as the sanitizer detects a problem" - no, we shouldn't recover differently/more safely with sanitizers enabled (don't want to create a language variant/encourage people to build incorrect programs with sanitizers and run them that way because "it works"))

Thu, Mar 23, 2:43 PM · Restricted Project, Restricted Project, Restricted Project
dblaikie added inline comments to D146662: DebugInfo: Rebuild dwp debug_info index column from v5 indexes more robustly.
Thu, Mar 23, 2:36 PM · Restricted Project, Restricted Project
dblaikie updated the diff for D146662: DebugInfo: Rebuild dwp debug_info index column from v5 indexes more robustly.

fix DWARF5 reference to DWARF4 for the test case

Thu, Mar 23, 2:35 PM · Restricted Project, Restricted Project
dblaikie added a comment to D146595: [clang] Add "transparent_stepping" attribute.

What about function overloading & namespaces? Is the debugger expected to figure out which bar function the DWARF/user is referring to?

The symbol name that's passed in the attribute is supposed to be the mangled name, so there shouldn't be any ambiguity to resolve.

Thu, Mar 23, 2:31 PM · debug-info, Restricted Project, Restricted Project, Restricted Project
dblaikie added a comment to D145609: [lldb] Change dwim-print to default to disabled persistent results.

The rationale is: dwim-print doesn't always use expression evaluation, it prefers to use frame variable where possible. In the future it could be expanded, for example to print register as well. Because dwim-print doesn't always use expression, there isn't always a persistent result.

Thu, Mar 23, 11:09 AM · Restricted Project, Restricted Project

Wed, Mar 22

dblaikie requested review of D146662: DebugInfo: Rebuild dwp debug_info index column from v5 indexes more robustly.
Wed, Mar 22, 2:32 PM · Restricted Project, Restricted Project
dblaikie added a comment to D138247: PR58819: Correct linkage and mangling of lambdas in inline static member initializers.

Ping

Wed, Mar 22, 1:31 PM · Restricted Project, Restricted Project
dblaikie added a comment to D146466: [clang] diagnose function fallthrough.

I'm concerned about the potential for false positives:

  • If a user doesn't mark a function noreturn, but it doesn't actually ever return.

Ok, but a user can fix that in their code. Also, I think we could possibly help by adding diagnostics for this. Unconditionally calling a noreturn function or having an obviously infinite loop should trigger such a diagnostic with note encouraging the user to mark their function noreturn as well?

Wed, Mar 22, 12:14 PM · Restricted Project, Restricted Project, Restricted Project
dblaikie added a comment to D146595: [clang] Add "transparent_stepping" attribute.

So looking at the DWARF spec for this - I see what it's going for, but yeah, I wouldn't want to use a textual representation either at the source or bitcode level, ideally... - perhaps this should just be implemented in LLVM when something's being lowered to a trampoline we could emit DWARF that describes that, without needing source annotations? Or are there situations where it depends on how the user wrote the code as to how we want the debugger to dehave?

Wed, Mar 22, 11:31 AM · debug-info, Restricted Project, Restricted Project, Restricted Project
dblaikie added a comment to D146595: [clang] Add "transparent_stepping" attribute.

Yeah, this seems a bit burdensome for both the frontend (encoding more information, the author has to duplicate the name of the implementation function so there's a risk they get out of sync?) and the debugger (is the debugger expected to jump directly and not call the implementation? Is it expected to continue into the implementation and break at the function? What if the function isn't called, or there's more than one call in the function, etc?)...

Wed, Mar 22, 11:26 AM · debug-info, Restricted Project, Restricted Project, Restricted Project

Tue, Mar 21

dblaikie added inline comments to D137882: [DWARFLibrary] Add support to re-construct cu-index.
Tue, Mar 21, 10:20 AM · Restricted Project, Restricted Project
dblaikie accepted D146231: [ADT] Add `llvm::range_size` function for generic ranges.

Looks good, thanks!

Tue, Mar 21, 9:33 AM · Restricted Project, Restricted Project

Mon, Mar 20

dblaikie added inline comments to D137882: [DWARFLibrary] Add support to re-construct cu-index.
Mon, Mar 20, 5:53 PM · Restricted Project, Restricted Project
dblaikie added inline comments to D144006: [DebugMetadata][DwarfDebug] Support function-local types in lexical block scopes (5/7).
Mon, Mar 20, 5:49 PM · Restricted Project, debug-info, Restricted Project, Restricted Project
dblaikie added inline comments to D137882: [DWARFLibrary] Add support to re-construct cu-index.
Mon, Mar 20, 5:45 PM · Restricted Project, Restricted Project
dblaikie added a comment to D146466: [clang] diagnose function fallthrough.

Yeah, we already have the return type warning and UBSan for the dynamic version of this check - seems OK? That a function lowers to unreachable isn't a bug - there's a ton of them in LLVM, quite intentionally as unimplemented functions that should never be called.

Mon, Mar 20, 5:41 PM · Restricted Project, Restricted Project, Restricted Project

Fri, Mar 17

dblaikie added inline comments to D137882: [DWARFLibrary] Add support to re-construct cu-index.
Fri, Mar 17, 3:20 PM · Restricted Project, Restricted Project

Thu, Mar 16

dblaikie added inline comments to D137882: [DWARFLibrary] Add support to re-construct cu-index.
Thu, Mar 16, 5:56 PM · Restricted Project, Restricted Project
dblaikie added a comment to D144337: Prevent line 0 instructions from dividing a lexical block into ranges.

No, my intent was not to use UnknownLocations as a proxy for CV, but I think it is safe not to fold line 0 instructions into a scope range when UnknownLocations is enabled. AFAIK, when UnknownLocations is enabled line 0 entries are emitted in the debug information. In my opinion, Folding these instructions when the switch is enabled could break stuff.

Thu, Mar 16, 5:42 PM · Restricted Project, Restricted Project
dblaikie added inline comments to D146231: [ADT] Add `llvm::range_size` function for generic ranges.
Thu, Mar 16, 12:37 PM · Restricted Project, Restricted Project
dblaikie added a comment to D146231: [ADT] Add `llvm::range_size` function for generic ranges.

Would std::distance suffice?

Thu, Mar 16, 9:29 AM · Restricted Project, Restricted Project

Wed, Mar 15

dblaikie accepted D144503: [ADT] Allow `llvm::enumerate` to enumerate over multiple ranges.
Wed, Mar 15, 10:37 AM · Restricted Project, Restricted Project, Restricted Project
dblaikie added a comment to D144844: [C++20] [Modules] Offer -fno-import-inter-module-function-defs to avoid duplicated compilation in modules.

I don't think of this as a performance regression for users though - this functionality's never really "shipped" so we get to choose what the baseline is.

Wed, Mar 15, 10:20 AM · Restricted Project, Restricted Project, Restricted Project

Tue, Mar 14

dblaikie committed rGf198c508109d: Fix split-dwarf-dwp-invalid test to be Windows-path-separator compatible (authored by dblaikie).
Fix split-dwarf-dwp-invalid test to be Windows-path-separator compatible
Tue, Mar 14, 5:51 PM · Restricted Project, Restricted Project
dblaikie added a comment to rG35fd37177b9b: llvm-symbolizer: Don't crash when referencing an invalid CU in a dwp file twice.

Is this worth a unit test?

Tue, Mar 14, 3:45 PM · Restricted Project, Restricted Project
dblaikie committed rG1fd9ba9a616b: Add missing test for 35fd37177b9b201f26390fe963767be548c8c2e9 (authored by dblaikie).
Add missing test for 35fd37177b9b201f26390fe963767be548c8c2e9
Tue, Mar 14, 3:44 PM · Restricted Project, Restricted Project
dblaikie updated subscribers of D144337: Prevent line 0 instructions from dividing a lexical block into ranges.

If you consider the attached test case (llvm/test/DebugInfo/X86/contiguous-lexical-block.ll) you can see that the lexical block is fragmented into ranges due to the presence of an instruction having line number 0. This I believe is a bug and If the same test case is run on windows, Visual Studio' can't reliably print variable "i" because CodeView does not provide a mechanism for describing a lexical block with more than one address ranges. In order to prevent the line 0 instructions from breaking up a lexical block, in this patch, we fold the line zero machine instructions into the range.

Tue, Mar 14, 1:34 PM · Restricted Project, Restricted Project
dblaikie added a comment to D146061: [ADT] Make llvm::is_contained call member `contains` or `find` when available.

This is also consistent with how new C++ range functions are defined, e.g., https://en.cppreference.com/w/cpp/ranges/size that calls member .size when available and does iterator subtraction otherwise.

Tue, Mar 14, 1:31 PM · Restricted Project, Restricted Project
dblaikie added a comment to D146061: [ADT] Make llvm::is_contained call member `contains` or `find` when available.

I also considered detecting member contains and triggering a
static_assert instead, but decided against it because it's just as easy
to do the right thing and call .contains.

I think I would prefer this option, so we keep the ability to distinguish O(1) contains from O(n) is_contained at a glance.

The way I was thinking about this is that usually the C++ standard specifies algorithms in terms of worst-case complexity and anything better is a bonus. In a completely generic context, as a user of is_contained my expectation would be that it's O(n) or better.

This situation is a bit more complicated because C++20 is added member contains functions to (unordered_)set/map, so a static assertion would cause compilation failures with some compiler flags and not the other ones. IMO this is not a huge deal but probably annoying because I'd expect the Hyrum law to apply and API usages of is_contained with these containers to creep in over time. The current patch makes this API instability problem better , as there'll be no need to have ways to essentially do the same thing.

Tue, Mar 14, 1:30 PM · Restricted Project, Restricted Project
dblaikie added a comment to D144844: [C++20] [Modules] Offer -fno-import-inter-module-function-defs to avoid duplicated compilation in modules.

Seem to recall @iains and others were concerned about the number of modules flags - this one I'd be inclined to suggest we shouldn't add if possible. If one way or the other is generally better - we should, at least initially, dictate that behavior entirely until someone can demonstrate divergent use cases that seem reasonable to support but must have different behavior here.

Agreed, [IMO, at least] we have a very large set of modules options, and expanding that should only be done if no sensible alternative can be found (we are already seeing users getting confused about which ones apply in each case).

A second point here - that (once we have the ability to generate object and PCM in the same compilation) that we will move to elide the function bodies for non-inline and non-dependent cases from the PCM, so that this problem will magically "go away" (to restore the current behaviour, we'd then be using some optimisation control to avoid the elision, I suppose).

Tue, Mar 14, 1:26 PM · Restricted Project, Restricted Project, Restricted Project
dblaikie accepted D146006: [ADT][mlir][NFCI] Do not use non-const lvalue-refs with enumerate.

Looks good, thanks!

Tue, Mar 14, 10:06 AM · Restricted Project, Restricted Project
dblaikie added a comment to D146061: [ADT] Make llvm::is_contained call member `contains` or `find` when available.

I also considered detecting member contains and triggering a
static_assert instead, but decided against it because it's just as easy
to do the right thing and call .contains.

I think I would prefer this option, so we keep the ability to distinguish O(1) contains from O(n) is_contained at a glance.

Yeah, that's fair - I'm OK with that too/see the value there. Only issue is in generic code where you might not know the type of a container.

Tue, Mar 14, 9:58 AM · Restricted Project, Restricted Project
dblaikie added a comment to D146061: [ADT] Make llvm::is_contained call member `contains` or `find` when available.

I also considered detecting member contains and triggering a
static_assert instead, but decided against it because it's just as easy
to do the right thing and call .contains.

I think I would prefer this option, so we keep the ability to distinguish O(1) contains from O(n) is_contained at a glance.

Tue, Mar 14, 9:57 AM · Restricted Project, Restricted Project
dblaikie added a comment to D144844: [C++20] [Modules] Offer -fno-import-inter-module-function-defs to avoid duplicated compilation in modules.

Seem to recall @iains and others were concerned about the number of modules flags - this one I'd be inclined to suggest we shouldn't add if possible. If one way or the other is generally better - we should, at least initially, dictate that behavior entirely until someone can demonstrate divergent use cases that seem reasonable to support but must have different behavior here.

Tue, Mar 14, 9:53 AM · Restricted Project, Restricted Project, Restricted Project
dblaikie accepted D146061: [ADT] Make llvm::is_contained call member `contains` or `find` when available.

Looks good

Tue, Mar 14, 9:47 AM · Restricted Project, Restricted Project

Mon, Mar 13

dblaikie committed rG35fd37177b9b: llvm-symbolizer: Don't crash when referencing an invalid CU in a dwp file twice (authored by dblaikie).
llvm-symbolizer: Don't crash when referencing an invalid CU in a dwp file twice
Mon, Mar 13, 5:52 PM · Restricted Project, Restricted Project
dblaikie accepted D145987: [ADT][NFCI] Do not use non-const lvalue-refs with enumerate in llvm/.

Looks alright - thanks! :)

Mon, Mar 13, 3:34 PM · Restricted Project, Restricted Project, Restricted Project
dblaikie accepted D143985: [DwarfDebug] Move emission of imported entities from beginModule() to endModule() (2/7).

@dblaikie thank you for looking at this! Hope this will be the final and successful iteration of the patchset.

Hmm - have you checked/could you check if debuggers (gdb and lldb at least) care about where a namespace-scoped imported entity appears in the DWARF? It looks like this patch would move namespace-scoped imported entities to the end of the DWARF, and I could imagine an implementation trying to do name lookup might try to consider only the imported entities that appear lexically before in the DWARF? (admittedly we haven't put enough information in the IR to get these things in the /correct/ place, but maybe before is more likely to be correct than after) - though of course the same bug might exist in function-local imported entities (& other function local names) - and similarly we don't have enough info or ways to insert things in the right order anyway...

I haven't consulted with the debuggers code itself, but a simple example showed that neither gdb nor lldb have problems with finding imported entities if the order gets changed:

Mon, Mar 13, 12:53 PM · debug-info, Restricted Project, Restricted Project
dblaikie added a comment to D144337: Prevent line 0 instructions from dividing a lexical block into ranges.

Could you describe why this relates to/depends on the "UnknownLocations" flag? I'm not following from the description - we could still describe these locations as being at line zero, while including them in lexical scopes to avoid fragmented ranges, I think?

The reason why we want to check for “UnknownLocations” flag is that this flag is used to check if we want to add line 0 entries in the line table (in DwarfDebug.cpp) and thereby controlling whether or not line zero instructions are represented in the debug information. If we don’t check for this flag and skip the line 0 entries while extracting lexical scopes there will be a mismatch in the line table entries and lexical scopes.

Mon, Mar 13, 12:50 PM · Restricted Project, Restricted Project
dblaikie added a comment to D144008: [DebugMetadata][DwarfDebug] Support function-local static variables in lexical block scopes (7/7).

@dblaikie

@krisb - how's this handle cases of old IR being used in a newer compiler/linker?

There is no backward compatibility, old IR may cause DwarfDebug crash (it asserts that old IR is no longer supported) or miscompilation.
Implementing AutoUpgrade seems to be quite easy, but as far as I can see almost all the changes in debug metadata before were done w/o backward compatibility, so I haven't considered it yet.

Mon, Mar 13, 11:06 AM · debug-info, Restricted Project, Restricted Project
dblaikie added a comment to D144007: [DwarfDebug] Move emission of globals from beginModule() to endModule() (6/7).

I guess this one's a bit clearer than the type one about why changing the order is important for getting local entities correctly scoped - local static is a global variable for the debug info and IR, so we need to emit the scopes before we'll know where to put the globals, etc.

The motivation for this patch is mostly the same as for types:

  • unify/simplify emission of local and non-local entities getting everything done in one place (in our case DwarfDebug::endModule()). Keeping 'non-local' globals emission in DwarfDebug::beginModule() (while 'local' globals should be emitted not earlier than DwarfDebug::endModule()) would make the implementation much more complicated as both 'local' and 'global' globals emission depends on GVMap.
  • make sure IR/debug metadata would not be changed after DWARF entities get created.
Mon, Mar 13, 11:04 AM · debug-info, Restricted Project, Restricted Project
dblaikie added a comment to D144005: [DwarfDebug] Move emission of types from beginModule() to endModule() (4/7).

Change seems OK - but might be nice to know a bit more about why this is helpful for correcting function-local entities?

Strictly speaking, moving emission of non-local types isn't required to get local types worked. This is mostly done for the sake of unification/simplification since we are stopping emitting anything in DwarfDebug::beginModule(). There is another motivation point, but it is a bit speculative: we may get IR (as well as debug metadata) changed after DwarfDebug::beginModule(), and if this happened, emitted debug info would be out-of-date. I've found such a case only once, and considered it rather a bug (see for details https://reviews.llvm.org/D113653). I'm not aware of any other cases, but it's still possible to change debug metadata after DwarfDebug::beginModule(), so it seems much safer to emit DWARF at a time when we do not expect any changes in IR (i.e. DwarfDebug::endModule()).

Mon, Mar 13, 11:02 AM · debug-info, Restricted Project, Restricted Project

Sat, Mar 11

dblaikie added a comment to D145803: [clang][DebugInfo] Emit DW_AT_type of preferred name if available.

Yeah, can't say this had occurred to me - but totally makes sense/reckon it's OK. Any reason to limit this to lldb? I'd expect it'd probably "Just Work(tm)" on any DWARF consumer?

Sat, Mar 11, 10:06 AM · Restricted Project, Restricted Project, Restricted Project
dblaikie added inline comments to D145805: [DWARFLinker][DWARFv5] Add support for DW_FORM_addrx*.
Sat, Mar 11, 9:57 AM · Restricted Project, Restricted Project

Fri, Mar 10

dblaikie updated subscribers of D144008: [DebugMetadata][DwarfDebug] Support function-local static variables in lexical block scopes (7/7).

Since this changes what IR goes where - are there concerns you folks have for auto-upgrade/cross compatibility?

Fri, Mar 10, 11:03 AM · debug-info, Restricted Project, Restricted Project
dblaikie accepted D144007: [DwarfDebug] Move emission of globals from beginModule() to endModule() (6/7).

I guess this one's a bit clearer than the type one about why changing the order is important for getting local entities correctly scoped - local static is a global variable for the debug info and IR, so we need to emit the scopes before we'll know where to put the globals, etc.

Fri, Mar 10, 11:01 AM · debug-info, Restricted Project, Restricted Project
dblaikie accepted D144005: [DwarfDebug] Move emission of types from beginModule() to endModule() (4/7).

Change seems OK - but might be nice to know a bit more about why this is helpful for correcting function-local entities?

Fri, Mar 10, 10:57 AM · debug-info, Restricted Project, Restricted Project
dblaikie added a comment to D143985: [DwarfDebug] Move emission of imported entities from beginModule() to endModule() (2/7).

& also - if we're moving local imported entities into the function-local retainedNodes list, do we need to change this code that should just be left constructing global/namespace-scoped imported entities? Perhaps we can leave this code/ordering alone?

Fri, Mar 10, 10:55 AM · debug-info, Restricted Project, Restricted Project
dblaikie added a comment to D143985: [DwarfDebug] Move emission of imported entities from beginModule() to endModule() (2/7).

Hmm - have you checked/could you check if debuggers (gdb and lldb at least) care about where a namespace-scoped imported entity appears in the DWARF? It looks like this patch would move namespace-scoped imported entities to the end of the DWARF, and I could imagine an implementation trying to do name lookup might try to consider only the imported entities that appear lexically before in the DWARF? (admittedly we haven't put enough information in the IR to get these things in the /correct/ place, but maybe before is more likely to be correct than after) - though of course the same bug might exist in function-local imported entities (& other function local names) - and similarly we don't have enough info or ways to insert things in the right order anyway...

Fri, Mar 10, 10:53 AM · debug-info, Restricted Project, Restricted Project
dblaikie accepted D143984: [DebugMetadata] Simplify handling subprogram's retainedNodes field. NFCI (1/7).
Fri, Mar 10, 10:47 AM · Restricted Project, debug-info, Restricted Project, Restricted Project

Thu, Mar 9

dblaikie added a comment to D144337: Prevent line 0 instructions from dividing a lexical block into ranges.

Could you describe why this relates to/depends on the "UnknownLocations" flag? I'm not following from the description - we could still describe these locations as being at line zero, while including them in lexical scopes to avoid fragmented ranges, I think?

Thu, Mar 9, 6:14 PM · Restricted Project, Restricted Project
dblaikie added a comment to D144503: [ADT] Allow `llvm::enumerate` to enumerate over multiple ranges.

I think it's the InputIterator (ForwardIterators must be InputIterators) requirement is the one that makes value returns not possible in practice because they're required to support i->m as being equivalent to (*i).m`, which isn't possible, as far as I recall/understand without backing storage to return a pointer to?

Oh, and ForwardIterator also says (on cppreference):

Let T be the value type of It. The type std::iterator_traits<It>::reference must be either
  T& or T&& (since C++11) if It satisfies LegacyOutputIterator (It is mutable), or
  const T& or const T&& (since C++11) otherwise (It is constant),
(where T is the type denoted by std::iterator_traits<It>::value_type)

David and I talked offline and this interpretation of the standard is probably the correct one. To conform to the iterator requirements, we would have to store something like std::optional<TupleOfReferences> inside zip_common and materialize it on the first dereference. This would be a proper fix for all zip functions, but it doesn't seem like any part of llvm relies on reference being an actual reference right now, so it doesn't seem like a blocker.

I will update all uses of for (auto &it : enumerate(...)) in a separate revision before landing this and leave a FIXME for the reference type issues.

I think its also fair to say that this is somewhat a historical mistake that was never properly followed (see std::vector<bool>) which is probably why this requirement was dropped in C++20 concepts for forward iterators: https://en.cppreference.com/w/cpp/iterator/forward_iterator

Thu, Mar 9, 3:59 PM · Restricted Project, Restricted Project, Restricted Project
dblaikie added inline comments to D145499: [DebugInfo][DWARF] Add doesFormBelongToClass function..
Thu, Mar 9, 3:31 PM · Restricted Project, Restricted Project

Wed, Mar 8

dblaikie accepted D144503: [ADT] Allow `llvm::enumerate` to enumerate over multiple ranges.

I don't totally object - though, for myself (I think this was discussed previously, but I don't fully understand the refutation/disagreement) I think I'd prefer to expose the compositions themselves and probably not have a special return type, so the end result is:

for (auto &[index, v1, v2] : zip(index{}, r1, r2))

And drop "enumerate" entirely? It doesn't seem like it adds enough - though I guess having a name for it is useful. *shrug*

Wed, Mar 8, 12:52 PM · Restricted Project, Restricted Project, Restricted Project
dblaikie added a comment to D144503: [ADT] Allow `llvm::enumerate` to enumerate over multiple ranges.

I think it's the InputIterator (ForwardIterators must be InputIterators) requirement is the one that makes value returns not possible in practice because they're required to support i->m as being equivalent to (*i).m`, which isn't possible, as far as I recall/understand without backing storage to return a pointer to?

Wed, Mar 8, 12:22 PM · Restricted Project, Restricted Project, Restricted Project
dblaikie added a comment to D145499: [DebugInfo][DWARF] Add doesFormBelongToClass function..

Sounds OK, though might be nice to avoid having these different ways of using (with/without DWARFUnit) and find some way to unify them (have some more general abstraction over DWARFUnit that can be provided by all users of this API or something)

something like this?

diff --git a/llvm/include/llvm/DebugInfo/DWARF/DWARFFormValue.h b/llvm/include/llvm/DebugInfo/DWARF/DWARFFormValue.h
index fa47621265f7..b13f5ffdf7f4 100644
--- a/llvm/include/llvm/DebugInfo/DWARF/DWARFFormValue.h
+++ b/llvm/include/llvm/DebugInfo/DWARF/DWARFFormValue.h
@@ -77,10 +77,7 @@ public:
   dwarf::Form getForm() const { return Form; }
   uint64_t getRawUValue() const { return Value.uval; }
 
-  /// Returns true if current value is of \p FC form class.
-  /// If current value does not have reference to original DWARFUnit
-  /// then \p DwarfVersion might be used to check form class of value.
-  bool isFormClass(FormClass FC, uint16_t DwarfVersion = 3) const;
+  bool isFormClass(FormClass FC) const;
   const DWARFUnit *getUnit() const { return U; }
   void dump(raw_ostream &OS, DIDumpOptions DumpOpts = DIDumpOptions()) const;
   void dumpSectionedAddress(raw_ostream &OS, DIDumpOptions DumpOpts,
@@ -352,6 +349,9 @@ toBlock(const std::optional<DWARFFormValue> &V) {
   return std::nullopt;
 }
 
+bool doesFormBelongToClass(dwarf::Form Form, DWARFFormValue::FormClass FC,
+                                 uint16_t DwarfVersion);
+
 } // end namespace dwarf
 
 } // end namespace llvm
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFFormValue.cpp b/llvm/lib/DebugInfo/DWARF/DWARFFormValue.cpp
index fd94c8a7ffbb..5ea0aa63c218 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFFormValue.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFFormValue.cpp
@@ -213,41 +213,8 @@ bool DWARFFormValue::skipValue(dwarf::Form Form, DataExtractor DebugInfoData,
   return true;
 }
 
-bool DWARFFormValue::isFormClass(DWARFFormValue::FormClass FC,
-                                 uint16_t DwarfVersion) const {
-  // First, check DWARF5 form classes.
-  if (Form < ArrayRef(DWARF5FormClasses).size() &&
-      DWARF5FormClasses[Form] == FC)
-    return true;
-  // Check more forms from extensions and proposals.
-  switch (Form) {
-  case DW_FORM_GNU_ref_alt:
-    return (FC == FC_Reference);
-  case DW_FORM_GNU_addr_index:
-    return (FC == FC_Address);
-  case DW_FORM_GNU_str_index:
-  case DW_FORM_GNU_strp_alt:
-    return (FC == FC_String);
-  case DW_FORM_LLVM_addrx_offset:
-    return (FC == FC_Address);
-  default:
-    break;
-  }
-
-  if (FC == FC_SectionOffset) {
-    if (Form == DW_FORM_strp || Form == DW_FORM_line_strp)
-      return true;
-    // In DWARF3 DW_FORM_data4 and DW_FORM_data8 served also as a section
-    // offset. If we don't have a DWARFUnit, default to the old behavior.
-    if (Form == DW_FORM_data4 || Form == DW_FORM_data8) {
-      if (U)
-        return U->getVersion() <= 3;
-
-      return DwarfVersion <= 3;
-    }
-  }
-
-  return false;
+bool DWARFFormValue::isFormClass(DWARFFormValue::FormClass FC) const {
+  return doesFormBelongToClass(Form, FC, U ? U->getVersion() : 3);
 }
 
 bool DWARFFormValue::extractValue(const DWARFDataExtractor &Data,
@@ -783,3 +750,36 @@ DWARFFormValue::getAsFile(DILineInfoSpecifier::FileLineInfoKind Kind) const {
   }
   return std::nullopt;
 }
+
+bool doesFormBelongToClass(dwarf::Form Form, DWARFFormValue::FormClass FC,
+                                 uint16_t DwarfVersion) {
+    // First, check DWARF5 form classes.
+    if (Form < ArrayRef(DWARF5FormClasses).size() &&
+        DWARF5FormClasses[Form] == FC)
+      return true;
+    // Check more forms from extensions and proposals.
+    switch (Form) {
+    case DW_FORM_GNU_ref_alt:
+      return (FC == DWARFFormValue::FC_Reference);
+    case DW_FORM_GNU_addr_index:
+      return (FC == DWARFFormValue::FC_Address);
+    case DW_FORM_GNU_str_index:
+    case DW_FORM_GNU_strp_alt:
+      return (FC == DWARFFormValue::FC_String);
+    case DW_FORM_LLVM_addrx_offset:
+      return (FC == DWARFFormValue::FC_Address);
+    default:
+      break;
+    }
+
+    if (FC == DWARFFormValue::FC_SectionOffset) {
+      if (Form == DW_FORM_strp || Form == DW_FORM_line_strp)
+        return true;
+      // In DWARF3 DW_FORM_data4 and DW_FORM_data8 served also as a section
+      // offset.
+      if (Form == DW_FORM_data4 || Form == DW_FORM_data8)
+        return DwarfVersion <= 3;
+    }
+
+    return false;
+}
Wed, Mar 8, 11:43 AM · Restricted Project, Restricted Project

Tue, Mar 7

dblaikie accepted D145489: [STLExtras] Use std::get in less_first,less_second to support more types.

Sounds good to me - wouldn't even mind if you wanted to implement a generic abstraction, less<N> and implement less_first/second as aliases of that type.

Tue, Mar 7, 8:35 AM · Restricted Project, Restricted Project
dblaikie accepted D145499: [DebugInfo][DWARF] Add doesFormBelongToClass function..

Sounds OK, though might be nice to avoid having these different ways of using (with/without DWARFUnit) and find some way to unify them (have some more general abstraction over DWARFUnit that can be provided by all users of this API or something)

Tue, Mar 7, 8:30 AM · Restricted Project, Restricted Project

Mon, Mar 6

dblaikie added a comment to D138247: PR58819: Correct linkage and mangling of lambdas in inline static member initializers.

GCC bug hasn't got much traction, so I pinged it.

Mon, Mar 6, 10:33 PM · Restricted Project, Restricted Project
dblaikie added a comment to D140478: [RISCV] For Dwarf v5, emit indices into .debug_addr for range list entries.

What does it mean for it to be per-function? Can it vary at all between functions in the same module? (if not, why is it per-function?)

It governs whether or not that function's text relocations are paired with R_RISCV_RELAX to tell the linker it can relax them.

Mon, Mar 6, 10:09 PM · Restricted Project, Restricted Project
dblaikie added a comment to D144503: [ADT] Allow `llvm::enumerate` to enumerate over multiple ranges.

Because the enumerate_result returned on dereference is a temporary, enumeration result can no longer be used through an lvalue ref.

Mon, Mar 6, 10:04 PM · Restricted Project, Restricted Project, Restricted Project
dblaikie accepted D145009: Split getCompileUnitFor{Data,Code}Address..

Yep, looks good. (interestingly, this implementation might be missing some functionality for Split DWARF - in a Split DWARF build I didn't see any perf improvement, because I think there was no perf degredation - because this feature isn't properly implemented for Split DWARF, I'd guess)

Mon, Mar 6, 9:44 PM · Restricted Project, Restricted Project
dblaikie accepted D145357: [SelectionDAG] Deprecate isNullValue and isAllOnesValue.

Sure, sounds good. (maybe document the history of how we ended up with duplicate functionality/different names for the same thing? (I haven't checked the history - if you introduced the new names, maybe link to where they were introduced and/or summarize why they were introduced, etc))

Mon, Mar 6, 9:06 PM · Restricted Project, Restricted Project
dblaikie added a comment to D145249: [TargetParser] Disallow Global Constructors.

@mehdi_amini can you explain what the original reasoning behind this restriction was for Support, and whether we need to make the same restriction for TargetParser now that it's split out?

Mon, Mar 6, 8:59 PM · Restricted Project, Restricted Project
dblaikie added a comment to D145256: [clang/Diagnostic] Use `optional` to disambiguate between a `StoredDiagMessage` that is not set vs set as empty string.

"But when would you have a completely empty diagnostic message", you ask dear reader?
That is when there is an empty "#warning" in code.

Mon, Mar 6, 8:50 PM · Restricted Project, Restricted Project
dblaikie added a comment to D145023: [memprof] Refactor tests to generate binaries and profiles on the fly..

FWIW, there's a drawback here: Now llvm-profdata is missing test coverage in the llvm subproject. Someone making changes would reasonably expect that "check-llvm" would be adequate to test their changes. So we do try pretty hard to keep tests for code in the same subproject as the code itself. (but yes, also the llvm subproject can't/shouldn't depend on compiler-rt)

Mon, Mar 6, 3:07 PM · Restricted Project, Restricted Project, Restricted Project
dblaikie added a comment to D144337: Prevent line 0 instructions from dividing a lexical block into ranges.

Does this need a flag? The patch describes doing this for CodeView? Could/should it be done unconditionally for CV?

Mon, Mar 6, 1:52 PM · Restricted Project, Restricted Project
dblaikie added a comment to D145258: [bazel] Don't alwayslink clang-tidy libraries.

I would've thought it was necessary to alwayslink them because maybe they use some kind of auto-registration system into clang-tidy, so without alwayslink your clang-tidy binary wouldn't link any of them in? But I could be wrong, maybe they're explicitly referenced in the clang-tidy binary?

There is no always-link in cmake as far as I know, so we should not need any in Bazel either in the LLVM project hopefully.

Mon, Mar 6, 7:35 AM · Restricted Project, Restricted Project

Fri, Mar 3

dblaikie added a comment to D145077: [clang][DebugInfo] Support DW_AT_LLVM_preferred_name attribute.

I originally was hoping we wouldn't have to introduce a new attribute for this, but it looks like there are legitimate concerns about the alternatives, so in that sense this looks good!

Out of curiosity, what were the other existing attributes that were considered? (would be curious to have them written down here to explain/add strength to the motivation to add a new extension attribute)

Fri, Mar 3, 1:28 PM · Restricted Project, Restricted Project
dblaikie added a comment to D145077: [clang][DebugInfo] Support DW_AT_LLVM_preferred_name attribute.

I originally was hoping we wouldn't have to introduce a new attribute for this, but it looks like there are legitimate concerns about the alternatives, so in that sense this looks good!

Fri, Mar 3, 1:27 PM · Restricted Project, Restricted Project
dblaikie added a comment to D145258: [bazel] Don't alwayslink clang-tidy libraries.

I would've thought it was necessary to alwayslink them because maybe they use some kind of auto-registration system into clang-tidy, so without alwayslink your clang-tidy binary wouldn't link any of them in? But I could be wrong, maybe they're explicitly referenced in the clang-tidy binary?

Fri, Mar 3, 1:13 PM · Restricted Project, Restricted Project
dblaikie added a comment to D145051: [ADCE] Only remove debug intrinsics if non debug instructions are removed.

I think what we can do instead is to mark MemorySSA as preserved if only debuginfo intrinsics have been removed. (This is safe, MSSA explicitly never tracks debuginfo intrinsics.)

Does everyone agree this is the way forward then? If so I can put up a patch doing that on Monday.

Fri, Mar 3, 1:11 PM · debug-info, Restricted Project, Restricted Project
dblaikie added a comment to D145199: [ELF] Mention section name for STT_SECTION in reportRangeError().

remove offset (since it's always zero for the STT_SECTION symbol)

Fri, Mar 3, 1:09 PM · Restricted Project, Restricted Project
dblaikie added a comment to D140478: [RISCV] For Dwarf v5, emit indices into .debug_addr for range list entries.

@dblaikie

As I already replied earlier, there is not enough context to reach the MachineFunction from the symbol or determine if relaxation is enabled for the given address range.

Then won't this make the wrong decisions if the value varies between functions/subtargets?

Also, at the moment, the -mrelax/-mno-relax affects the entire compilation unit for the RISC-V.

Then wouldn't these be target features, rather than subtarget features? My understanding is that subtarget features generally can vary by function, whereas target features cannot.

Even if the command line flags don't support it, we could probably make a simple IR example with different relaxation per function? & so we probably should be able to handle that case.

It's a per-function feature that has implications for other functions in the same section (deleting code in one function at link time will shunt around the code in the rest of the section).

Fri, Mar 3, 1:04 PM · Restricted Project, Restricted Project

Thu, Mar 2

dblaikie added a comment to D140478: [RISCV] For Dwarf v5, emit indices into .debug_addr for range list entries.

@dblaikie

As I already replied earlier, there is not enough context to reach the MachineFunction from the symbol or determine if relaxation is enabled for the given address range.

Thu, Mar 2, 3:41 PM · Restricted Project, Restricted Project