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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
