[PATCH] D126059: [Debuginfo][DWARF][NFC] Add paired methods working with DWARFDebugInfoEntry.

Mon May 23 16:15:00 PDT 2022

clayborg added a comment.

In D126059#3532473 <https://reviews.llvm.org/D126059#3532473>, @avl wrote:

> In D126059#3532264 <https://reviews.llvm.org/D126059#3532264>, @clayborg wrote:
>
>> A 1 to 2 percent improvement is probably going to go away if the asserts are enabled when it has to do the check each time. If would still hesitate to add this kind of API as it will encourage others to use the APIs. I would be ok with it if the methods are protected or private and if we can add the DWARFLinker as a friend class to allow access to these performant but dangerous APIs only in certain situations that really require it. I really want to avoid most people from being able to use these APIs if possible to make sure we don't end up with bugs. We had many many bugs that stemmed from mismatching DWARFUnit + DWARFDebugIntoEntry before the DWARFDie class was created, mostly when doing cross CU references or in Fission cases where you have the skeleton CU vs the actual DWO CU + a DWARFDebugInfoEntry that comes from the actual DWO file. So there is a real danger to the safety and correctness of the code here.
>
> if all of these bugs could be caught by assertions then the problem probably does not look too severe?
> Running debug version of the application will show the problems.
>
> Anyway, if we do not want these methods to be used by others - I am OK to make these methods protected and make DWARFLinker the only one using them.

I personally would be fine if there were guardrails on this feature where we make them protected, though I am not a code owner the DWARF parser in LLVM, so some input from the LLVM DWARF community would be good to verify they agree with this direction.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126059