[PATCH] D113741: [RFC][DwarfDebug][AsmPrinter] Support emitting function-local declaration for a lexical block

Tue Mar 29 12:57:34 PDT 2022

dblaikie added a comment.

In D113741#3348795 <https://reviews.llvm.org/D113741#3348795>, @krisb wrote:

> In D113741#3343849 <https://reviews.llvm.org/D113741#3343849>, @dblaikie wrote:
>
>> In D113741#3343683 <https://reviews.llvm.org/D113741#3343683>, @krisb wrote:
>>
>>> In D113741#3343496 <https://reviews.llvm.org/D113741#3343496>, @dblaikie wrote:
>>>
>>>> I'm not immediately following why a list of CUs is needed/why we'd create the variable in more than one CU - could you explain that?
>>>
>>> Well, this is because we may have a parent subprogram emitted in more than one CU (in the case of split dwarf), and we do not know in which one. 
>>> Here are the situations I see (hope I got them right):
>>> (1) If split dwarf disabled, we have a single list of subprogram's abstract origins and can reference them between CUs. So, we'll emit a concrete definition and its abstract origin (any existing) in the same CU. In this case, we have only one CU.
>>> (2) If we have split dwarf enabled, and `shareAcrossDWOCUs()` is false (it's false by default), every CU will maintain its own abstract origin's list. In this case, we cannot have references between CUs, and if we have cross-CU inlining, we'll create:
>>>
>>> - concrete def -> in the CU the subprogram is defined,
>>> - abstract origin -> in every CU the subprogram is inlined. For all local entities we should follow the same logic.
>>
>> Do you have a test case for this (sorry, I realize it might be one of the test cases in the patch - I haven't looked/would appreciate some pointers)? My recollection is that without shareAcrossDWOCUs we should probably be only emitting one CU if using Split DWARF - Split DWARF doesn't really support multiple CUs in a single .dwo file (& we only support emitting a single .dwo for a given .o file) - but I guess maybe we do in full LTO mode, even if we don't do it in ThinLTO mode (ThinLTO's the only thing I've been especially concerned with/the only mode we use at Google)?
>
> Okay, let's summarize this a bit.
>
> Statement 1. The problem with the example you provided above was because this patch made subprograms and their local "global" entities emitted in different CUs in the case of split DWARF. This is a bug, for sure.

Is that bug now addressed in the proposed patch & with tests to demonstrate it's fixed?

> Statement 2. Currently, there is nothing in the code that restricts emitting multiple CUs in a single .dwo (whether for FullLTO or for manually linked modules (as in the tests) we will generate more than one CU per .dwo).

I believe `DwarfDebug.cpp:SplitDwarfCrossCuReferences` is intended to disallow this functionality, so far as I can tell/remember?

> All my assumptions and conclusions are based on the code and existing regression tests. Unfortunately, I'm not aware of the real situation of split DWARF or split DWARF inlining support (and I couldn't find anything related in the docs either).
>
> Having this, I think we cannot assume that a subprogram is guaranteed to be emitted only in one CU, unless we explicitly disallow the case with multiple CUs per .dwo. DWARF standard seems to assume only one CU per .dwo (but I couldn't find any explicit wording about restrictions on the number of CUs in a .dwo). However, it looks like GDB is okay and works fine with more than one CU in a .dwo. It'd be good to have this situation clarified. But it seems to be out of the scope of this patch. Do you have any thoughs on how to proceed with this?

One of the issues is that the dwp file format describes an index by CU hash (DWO ID) - it's unclear what it would mean to have multiple entries that overlap (because a single region contains multiple CUs) and where the region doesn't start with the CU for the lookup (because it's some region with multiple units).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113741