User Details
- User Since
- Oct 8 2012, 9:19 AM (546 w, 5 d)
Wed, Mar 29
Tue, Mar 28
"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)
Mon, Mar 27
Rename VariableTemplate classification to TemplatedVariable to match ABI wording
Remove StaticDataMember and classify static member variables of templates as variable templates
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.
Thanks!
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.
Looks good, thanks!
Sat, Mar 25
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.
Thu, Mar 23
(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"))
fix DWARF5 reference to DWARF4 for the test case
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.
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.
Wed, Mar 22
Ping
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?
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?)...
Tue, Mar 21
Looks good, thanks!
Mon, Mar 20
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.
Fri, Mar 17
Thu, Mar 16
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.
Would std::distance suffice?
Wed, Mar 15
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.
Tue, Mar 14
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.
Looks good, thanks!
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.
Looks good
Mon, Mar 13
Looks alright - thanks! :)
Sat, Mar 11
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?
Fri, Mar 10
Since this changes what IR goes where - are there concerns you folks have for auto-upgrade/cross compatibility?
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.
Change seems OK - but might be nice to know a bit more about why this is helpful for correcting function-local entities?
& 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?
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...
Thu, Mar 9
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?
Wed, Mar 8
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*
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?
Tue, Mar 7
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.
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)
Mon, Mar 6
GCC bug hasn't got much traction, so I pinged it.
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.
Because the enumerate_result returned on dereference is a temporary, enumeration result can no longer be used through an lvalue ref.
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)
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))
"But when would you have a completely empty diagnostic message", you ask dear reader?
That is when there is an empty "#warning" in code.
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)
Does this need a flag? The patch describes doing this for CodeView? Could/should it be done unconditionally for CV?
Fri, Mar 3
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?