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

Mon Feb 28 05:29:20 PST 2022

krisb added a comment.

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

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?

>> (3) SplitDwarfInlining adds skeleton units to this list. Basically, we do not need to emit any local entities for skeleton units in this case, but I just reused already existing logic around `includeMinimalInlineScopes()`.
>>
>> The reproducer you provided is about the case (2). We tried to emit the imported entity in a CU, where its parent subprogram was defined, but the subprogram itself was emitted in the CU it was inlined.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113741