[Lldb-commits] [PATCH] D131705: Don't create sections for SHN_ABS symbols in ELF files.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: lldb/test/API/python_api/absolute_symbol/TestAbsoluteSymbol.py:28-29
+            and this caused problems as the new sections could interfere with
+            with address resolution. Absolute symbols' values are not addresses
+            and should not be treated this way.
+
----------------
labath wrote:
> As I said in the overall comment, I cannot agree with this statement without reservations. 
> 
> And in any case, I think prose like this is better left for the commit message. The test should just state what it does (although that's fairly obvious here), not what it used to do.
I can remove this part of the comment.


================
Comment at: lldb/test/API/python_api/absolute_symbol/TestAbsoluteSymbol.py:76-80
+        # Make sure no sections were created for the absolute symbol with a
+        # prefix of ".absolute." followed by the symbol name as they interfere
+        # with address lookups if they are treated like real sections.
+        for section in module.sections:
+            self.assertTrue(section.GetName() != ".absolute.absolute")
----------------
labath wrote:
> This check isn't particularly useful, since you've completely deleted the code which created those sections. If something similar is ever reintroduced, it's quite possible it would use a different section name, and so it wouldn't catch this.
> 
> I don't think it's necessary, but if you want, I think it'd be better to run something like `image lookup -a 0xffffffff80000000` and check that it does not produce any matches, as that's what you're really interested in.
Sounds good as long as we don't start trying to resolve the symbol as mentioned


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131705/new/

https://reviews.llvm.org/D131705