[llvm] Make DWARFUnitVector threadsafe. (PR #71487)

Greg Clayton via llvm-commits llvm-commits at lists.llvm.org
Tue Nov 7 10:45:19 PST 2023


clayborg wrote:

> Could you check the history and describe why the lazy feature was added (I'd guess I probably added it for symbolizing performance, or for other DWP usage performance) and why this change ensures those needs are still addressed?

It will regress on the performance gain that they added, but this change, while it got preformance, broke the API contract that is expected from DWARFUnitVector. If you search for all of the calls to `getDWOUnits(...)`, nearly all of the calls specify false for `Lazy`. So if you let the .dwp file initialize its DWARFUnitVector with `Lazy = true`, and then call `DWARFUnitVector::getUnitForIndexEntry(...)`, and then call any function that actually wants all of the units, like calling this function:
```
  unit_iterator_range dwo_info_section_units() {
    DWARFUnitVector &DWOUnits = State->getDWOUnits();
    return unit_iterator_range(DWOUnits.begin(),
                               DWOUnits.begin() + DWOUnits.getNumInfoUnits());
  }

  const DWARFUnitVector &getDWOUnitsVector() {
    return State->getDWOUnits();
  }

```
You will get a partial list of only the units that were manually added by the call to `DWARFUnitVector::getUnitForIndexEntry(...)`.  So the API really doesn't work, or if it does work, it only works for special cases if used in just the righy way.

> > not thread safe and causes crashes
> 
> Does it cause crashes in non-multithreaded situations? If not, then it may be hard to justify pessimizing the non-multithreaded usage to fix the threaded usage without some more nuance about how to address these different needs.

Yeah. I can modify this change to specify `Lazy = true` only if multi-threading is not enabled, but the class itself doesn't work as it currently is created. the code would need to be modified so if the user needs all of the units, (they call `DWARFContextState:: getDWOUnits(Lazy = false)`, then it should fully populate the list. 

So if I want to just fix the bug where llvm-gsymutil crashes I can do so easily, but this API isn't safe to use in all situations as I mention above. Let me know what approach you would prefer



https://github.com/llvm/llvm-project/pull/71487


More information about the llvm-commits mailing list