- User Since
- Aug 27 2019, 11:05 PM (49 w, 19 h)
How does the start scope compare to the location range of the variable, and the scope range of the enclosing scope?
DW_AT_start_scope value in contiguous scope case is offset from start of the Lex Block(i.e low_pc of enclosing scope).
Sorry for the confusion/churn. This is the resultant DWARF(for the variable of interest) for this test case.
0x0000007a: DW_TAG_variable **DW_AT_location (0x00000000: [0x0000000000401178, 0x00000000004011c1): DW_OP_breg6 RBP-24)** // This is what causing increased in location list(even at "-O0 -g") DW_AT_name ("Local") DW_AT_decl_file ("/Scope.c") DW_AT_decl_line (7) DW_AT_type (0x0000008f "int") DW_AT_start_scope (0x00000017)
LGTM(at least the part I reviewed). But it would be good to wait for @kiranchandramohan and IIRC @ftynse has contributed a lot in GPU dialect. So it would be nice to have there views/thoughts also.
Thanks for your patience :)
Mon, Aug 3
Sun, Aug 2
It's not just endiannes that is causing trouble to PPC target. Looking at a comment regarding PPC target's long double representation in AsmPrinter.cpp
// PPC's long double has odd notions of endianness compared to how LLVM // handles it: p goes first for *big* endian on PPC.
This patch does not handle this, I'll see if we can fix this up without too many changes, otherwise I'll file a new review covering all scenarios endianess and this target specific.
Sat, Aug 1
Fri, Jul 31
Please ignore these test cases dwarfdump-dataLocationVar.ll and dwarfdump-dataLocationExp.ll. These were generated with flang at the time when it was having bug of emitting dummy LexicalBlock. I've an approved PR in flang for this, once merged I'll correct these test cases(separately).
Thu, Jul 30
Wed, Jul 29
Thu, Jul 23
There was another failure in WebAssembly debug info test case caught by build-bot. That's also fixed with commit id mentioned.
Once the change stabilizes, I plan to do a quick refactoring(wherever applicable WRT this change).
Wed, Jul 22
Temporarily reverted in 9d2da6759b4d05d834371bcaaa8fc3d9b3385b18.
Due to failing/assertion in test case in Sparc Backend.
Fixed By: https://reviews.llvm.org/D84278
Fixed By: https://reviews.llvm.org/D84278
Fixed by: https://reviews.llvm.org/D84278
I think I'd be happy with emitting a warning to llvm::dbgs(), or an assembler comment (if that is available here, not sure).
Here, I've used LLVM_DEBUG so that it won't hurt any thing. This seems Okay to you ?
Addressed @aprantl review comments. Thanks for this!
For cases other than those gracefully handled(`float, double, long double) for DW_OP_implicit_value I'm considering a hard error sort of assert/llvm_unreachable or 2nd option could be just bail out early.
Thank you so much @dblaikie for this. Admittedly, for all the direction I've been digging, missed this one :)
The catch here is that getDwoLineTable(U)->getFile(... this will enable the line table through MCDwarf interface, so we don't have/need to rely in DwarfTypeUnit interface for enabling line table.
Anyways, I'm planning to abandon all the revisions associated with this. IMHO D82084 is also not needed(since you did it without touching that) so we can leave it until there is actual need/situation ?
Addressed review comments by @aprantl, Thanks for this.
- Edited the summary to reflect the intention.
- This patch switches to DW_OP_implicit_value for all the floating point constants float, double and long double, for demonstration purposes, considering 1 byte space saving(discussed above).
- Changes are still a bit conservative, in a sense emission of DW_OP_implicit_value is guarded with additional check for debugger tunning.
This will ensure that, we didn't break any existing machinery/consumers.
Mon, Jul 20
Sat, Jul 18
Tried one more time disregarding both the approach followed previously.
This time finally worked out(in a much cleaner way).
It exposes getDwoLineTable private method. But IMO that's needed
anyways, since it will used/needed subsequently in D81476.
Fri, Jul 17
@aprantl @probinson could you folks please provide a quick comment on this as a whole ?
Meanwhile, I also have a patch(locally) which switches to DW_OP_implicit_value for all the floating point constants float, double, long double working fantastic. For correctness checked that executables are debuggable with GDB.
Thu, Jul 16
@dblaikie and other reviewers, these changes seems Okay to you folks ?
Tue, Jul 14
Mon, Jul 13
Thanks @kiranchandramohan for reviewing this.
- Addressed nit comments.
- updated commit message accordingly.
Sat, Jul 11
Sure, I understand the need having specific name(and my mistake of choosing a generic name addImplicitValue for the purpose of this patch). I've noted down and work on your inputs/concerns and revise once @aprantl and @probinson also have a look on it. Thanks again for your inputs :)
Thanks @dblaikie for your feedback!
Not sure it's worth committing such a narrow implementation - might be worth a bit of generalization even in this first patch?
This operation(as you might have noticed) has limited usage, since DW_OP_stack_value and friends fits in other cases very well. This is specifically made to cater needs(such as in this case) where the variable size is bigger than (size of address). This will cater all the needs in math(HPC) based applications where usage of long double is ubiquitous.
looks to be very specific to long double right now (but without any assertions, API features, or comments to enforce that restriction) - and in the DwarfDebug.cpp caller, which could presumably be used for all constant values, maybe (not sure if that would be a good thing or not - haven't looked at the alternatives, etc))
Yes, that's very specific and you'll notice that it has very strict enforcement requirements isConstrantFP followed by if (AP.getDwarfVersion() >= 4 && RawBytes.getBitWidth() == 80)(making sure we don't mess up existing infra).
For the documentation/comments part, admittedly I didn't put any behavior/requirement specifics in the declaration, However the definition part is fully documented and self-explanatory(IMO). Should we put some brief comments there(declaration part) too ?
Fri, Jul 10
Thu, Jul 9
Apologies for If I'm pinging too frequently :) . Gentle Ping to all reviewers again! :)
Tue, Jul 7
Jul 5 2020
Gentle Ping :)
Jul 2 2020
This is the nature of this process we're on. We're being asked to upstream tiny bits with each diff, the smaller the better. The reality is that however one slices it, the code will always be interrelated. In order to keep the diffs small, some sort of compromise must be made
How about series/chain of patches filed together with dependencies[Parent-Child] specified ? This could help in review/tracking process.
Jul 1 2020
Do we really need this ?, Please have a look at https://sourceware.org/legacy-ml/gdb-patches/2017-02/msg00528.html
IMO, it would be unwise to commit it till the discussion thread in D82975 converges ?
When you say 'by default' - do you mean by default when the user requests macro debug info (via -fdebug-macro) or by default without any extra flag?
& what does GCC do? Does it have a way to emit the standard debug_macinfo in v4 and below? Or does it always emit the debug_macro GNU extension?
Jun 28 2020
Jun 26 2020
@dblaikie Are you Convinced/Okay with explanation and the overall changes.
Overall this seems great Thanks! I have one minor concern:
This patch seems to do 2 things: Support for nested parallel regions(which was crashing earlier) and some infrastructure change(introducing AllocBuilder..).
I'm not sure of this, but is it possible to separate these as 2( or more) patches ? 1 for Nested parallel region support and other patch as a infrastructure change ?
Jun 19 2020
Jun 18 2020
Jun 15 2020
Thanks @dblaikie for reviewing this!
Ping @ all reviewers!
LGTM, Thanks! Please wait for other reviewers, in case if they've any comments.
Jun 10 2020
Addressed @probinson review comments. Thanks for this!
Jun 9 2020
I've raised another review D81476 for addressing this. Will revise this afterwards.
Jun 1 2020
I don't think this field (debug_line_offset) is optional if the debug_macro.dwo section contains DW_MACRO_start_file: "If a DW_MACRO_start_file entry is present, the header contains a reference to the .debug_line section of the compilation."
Yes, but spec seems a bit ambiguous in stating this. "If zero, that field is omitted. Pg. 166, line 14-16", Not explicitly stating the fact/case where it should be always present.
And spec also doesn't say anything about explicit presence of debug_line.dwo when debug_macro.dwo is present, although it seems it should be present.
Added corrected test case.