- User Since
- Aug 17 2017, 6:34 AM (108 w, 6 d)
Thu, Sep 12
Wed, Sep 11
Please note, this patch is not for integrating. This is for illustrative purpose for the thread on llvm-dev. http://lists.llvm.org/pipermail/llvm-dev/2019-September/135068.html
Tue, Sep 10
Thu, Sep 5
Wed, Sep 4
Tue, Sep 3
Sun, Sep 1
My apologies for the delay. I was out of the office and I couldn 't finish it sooner.
Wed, Aug 21
Tue, Aug 20
So I guess my question is, where are loads for the SROAed allocas generated?
Aug 19 2019
OK, I will modify testcase to be single pass.
Aug 16 2019
I implemented the solution which avoids lowering dbg.declare for structures. Please check updated patch. Note, I marked two tests as XFAIL since they check for lowering which is not done with this patch. Though it looks to me that it would be better to make dbg.addr to work. I am thinking of changing LowerDbgDeclare in such a way that it would produce dbg.addr and dbg.value instead of dbg.value only. i.e. dbg.addr would be generated for cases when dbg.value with memory operand is generated currently...
Aug 13 2019
Folks, I would like to ask a question related to this review and DWARF standard:
Aug 8 2019
Jul 22 2019
Converting a dbg.value into a dbg.declare implicitly extends the duration ("lifetime" in the docs ) of the variable location from "until the next dbg.value" to "the entire scope". Consider what happens if we make the modifications here  to your test case. Imagine an IR producer that wants to temporarily bind some variable to a field of 'result' (the dbg.value added with DW_OP_deref), and later assigns '1' to the variable with the second dbg.value, to cover the rest of the function.
added check for aggregate dbg.value.
Jul 16 2019
@jmorse Thank you for this case and explanations. I will evaluate it more and adapt fix accordingly.
Jul 15 2019
addressed style issues. created empty expression for dbg.value case.
Jul 12 2019
Jul 11 2019
Thank you, I apologize for impatience.
Jul 8 2019
Jul 5 2019
removed whitespaces, added handling x86 ILP32 case.
Jul 4 2019
@jhenderson Thank you for pointing this out! Actually --verify complains about errors in both cases(with the fix and without the fix) for the testcase from this review:
Jun 26 2019
@MaskRay Hi MaskRay, PRETEND is a connected but separate issue. I agree it would be good to implement some kind of the PRETEND/CB_PRETEND logic in lld. But it does not make obsolete this patch. implementing of PRETEND logic answers the question "how to select proper debug info for COMDAT section". That patch answers the question "How to avoid overlapping address ranges in case debug info referenced to deleted code presented". This patch makes sense not only for PRETEND. Thus, I do not agree with "If we have PRETEND, we will not need the -2 hack in relocateNonAlloc". Let`s not mixup issues.
Jun 25 2019
Jun 14 2019
@echristo Hi Eric, Could check this revision, please ? Is that patch Ok for you ?
Jun 6 2019
Jun 4 2019
@echristo What is your opinion on this patch ? Is it worth to integrate it ?
Jun 3 2019
following performance results with and without this patch. they are equal :
Jun 2 2019
ping. @ruiu Do you think it would be useful to integrate this patch ?
May 31 2019
cleaned up tests.
May 30 2019
May 27 2019
deleted redundant tests.
May 22 2019
May 21 2019
deleted redundant check.
May 20 2019
limit resolving relocations to UINT64_MAX-1 value for only debug sections, as requested.
addressed comments: added check for WZR/XZR registers, changed test to run only LiveDebugValues pass.
In general it is not possible to handle zero case properly in lldb. It does not have information which address range is correct.
if platform allows section to contain code starting from zero then it is not possible to detect good and bad address range:
May 17 2019
I do agree that this patching should be limited to debug sections though.
Other consumer sections might have other rules regarding special values etc, and be working on the assumption of patching to 0 is the best plan.
actually other DWARF consumers are not happy.
May 16 2019
May 12 2019
May 7 2019
You are the one who propose to change the status quo and use UINT64_MAX-1.
The burden of proof is on your side. You have the obligation to collect evidence from
approaches taken by various DWARF consumers to prove the newly introduced special value wouldn't cause problems
May 6 2019
I've some previous discussions PR37212 PR37891 etc. -2 or -1 may be benign to some non-allocated sections like .debug.*, but may not be good to others.
May 5 2019
Apr 29 2019
Excuse me, I did not realize that those were questions. Please check my answers :
Apr 22 2019
Apr 21 2019
Apr 15 2019
Existing tools have learned various heuristics to avoid 0 DW_AT_low_pc collision, but I am not sure what they'd do if those relocations resolve to -1. llvm-symbolizer is certainly good (I have checked a few >examples - I'm learning it recently), but what about other tools?
@ruiu What do you think about using UINT64_MAX-1 as address to which relocations for deleted sections would be resolved ? Does it make sense ?
Apr 12 2019
@MaskRay Hi, Since you changed status of review into "Needs Revision" - What changes do you think should be done ?
Apr 10 2019
Apr 9 2019
__libc_csu_init is in libc.a(elf-init.o) and has no debug info. It should symbolize to ?? but not_used.cpp is incorrectly returned. ?? -> not_used.cpp
I'm not sure if making zero DW_AT_low_pc special for executable/DSO in DWARFDebugAranges is a good idea.
I think the current handling in lld is quite good, no need to special case on if (!Sym.getOutputSection()). The same problem can be reproduced for ld.bfd and gold.
I thought that address 0 is a good candidate to represent an invalid address, as nullptr is usually represented by address 0. I wonder why we want to use UINT64_MAX-2 instead of 0.
Apr 8 2019
Apr 3 2019
@rocallahan Would you mind to share the performance results of that patch, please ? Similar to above table for rusoto test, but with timings ...
Apr 2 2019
Mar 29 2019
added indentation and corrected sections string.
Mar 28 2019
addressed all comments, except indentation one, which I did not understand yet.
addressed all comments.
I think at this point the "debug info cabal" is more in favor of fragmenting the DWARF emitted by the compiler, so that it can be gc'd in tandem with the related functions.
Mar 25 2019
abandon revision because of already integrated duplicated revision https://reviews.llvm.org/D59490