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

Tue Aug 16 08:09:19 PDT 2022

labath added a subscriber: dsrbecky.
labath added a comment.

I have somewhat mixed feelings about this.

On one hand, I was never too happy with how we're creating these funny extra sections, so I can't say I would be sad to see that go. OTOH, I can't really agree with the assertion that these symbols do not represent "addresses".  All the ELF spec says about them is that they are not subject to relocation. That makes them rather ill-suited for describing the locations of objects in the usual scenarios. However, these symbols can be (and, I believe, are) used in the embedded world for describing objects that are architecturally defined to reside at a particular memory address (the 16-bit dos/bios world was full of those). However, one can imagine something similar being done in userspace as well (for example, if the kernel provides some data at a fixed virtual address). We envisioned something similar when we added support for this. In our case, the object/function in question was generated by a JIT, which knew the exact address at which it placed the jitted code.

However, AFAIK, that project never materialized, and we are not relying on these symbols for that. Tagging @dsrbecky to confirm. Assuming that's the case, and given that gdb does not handle these symbols in this way either (at least, I haven't been able to make it do that), I think we can go forward with removing them.

================
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.
+
----------------
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.

================
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")
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131705