- User Since
- Aug 17 2017, 6:34 AM (100 w, 2 d)
Tue, Jul 16
@jmorse Thank you for this case and explanations. I will evaluate it more and adapt fix accordingly.
Mon, Jul 15
addressed style issues. created empty expression for dbg.value case.
Fri, Jul 12
Thu, Jul 11
Thank you, I apologize for impatience.
Mon, Jul 8
Fri, Jul 5
removed whitespaces, added handling x86 ILP32 case.
Thu, Jul 4
@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:
Wed, Jun 26
@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.
Tue, Jun 25
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
Mar 23 2019
It seems you indented the description by 2. That may be why the revision is not automatically closed..
Mar 22 2019
Please move this down at least far enough that it doesn't involve separately
computing the file name and opening the file. Where it's a "well, if you didn't
specify a section index, we'll use the first section that covers this address".
updated tests as requested.
What does GDB make of -1/-2???
updated tests and addressed comments.
Mar 21 2019
addressed comments. added tests for 32 bit.
Does this do the right thing for a 32-bit target? 64-bit is common, but not universal.
Mar 20 2019
This seems like it might be worth a broader discussion about these sort of cases - probably llvm-dev,
maybe with some DWARF folks (though the usual LLVM debug info cabal (myself, @aprantl, @probinson,
@JDevlieghere, and @echristo) is probably sufficient to get a rough idea of what might be a good general approach).
Is this logically correct? I wonder what is the point of emitting a relocation section pointing to a nonexistent section.
- used UINT_MAX-1 as value for relocations referenced deleted section
- changed title of review
- updated tests
- I tried to use UINT64_MAX-1 as a address value for lost debug info :
Mar 19 2019
What do other linkers (gold and gnu binutils, for instance) do in this case?
I am not sure about using grep and sed in the test. I see it used in other parts of llvm but not inside lld.
Thanks for fixing this! Please check comments.
Mar 16 2019
Since that is compile time issue - not neccessary to delay it for long time. LGTM.
Mar 15 2019
David, there is a connected review - https://reviews.llvm.org/D59189 It cures compilation error after D58194 Could you take a look on it please ?
I think that change is OK and it is good to do in spite of decision made in this review.
Mar 14 2019
In case Undef specified then there could be several sections with matching address ranges.
It seems to me that DWARFDebugLineTable improper place to make such a decision - which of them should be taken as a result.
This in fact high level decision, not low level.