[PATCH] D113142: [CodeView] Properly handle a DISubprogram in getScopeIndex.

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: llvm/test/DebugInfo/COFF/fortran-contained-proc.ll:25
+;
+; CHECK: S_GPROC32_ID [size = {{.*}}] `IF_TEST`
+; CHECK-NEXT: parent
----------------
cchen15 wrote:
> rnk wrote:
> > I think the test should look at IF_TEST_ip_SUB, since that's the nested routine, not the main routine.  The nested routine should use the main routine as its parent scope, right?
> IF_TEST (the parent scope of SUB) is the subroutine I'd like to check here, because it is the one that getScopeIndex is called with, when the FuncId for SUB is being looked up in getFuncIdForSubprogram. And the test here is to ensure that IF_TEST points at the right type (two lines below).
> 
> That being said, your question prompted me to look at the rest of the dump a bit closer. Here are some observations:
> 1. SUB is inlined in the test. The S_INLINESITE sits nicely inside S_GPROC32_ID for IF_TEST. The type for that S_INLINESITE points at a LF_FUNC_ID whose parent scope points at the LF_FUNC_ID of IF_TEST.
> 
> ```
> 0x1000 | LF_ARGLIST [size = 8]
> 0x1001 | LF_PROCEDURE [size = 16]
>          return type = 0x0003 (void), # args = 0, param list = 0x1000
>          calling conv = cdecl, options = None
> 0x1002 | LF_FUNC_ID [size = 20]
>          name = IF_TEST, type = 0x1001, parent scope = <no type>
> 0x1003 | LF_FUNC_ID [size = 16]
>          name = SUB, type = 0x1001, parent scope = 0x1002
> <elided>
>        0 | S_GPROC32_ID [size = 48] `IF_TEST`
>            parent = 0, end = 0, addr = 0000:0000, code size = 69
>            type = `0x1002 (IF_TEST)`, debug start = 0, debug end = 0, flags = none
>        0 | S_FRAMEPROC [size = 32]
>            size = 40, padding size = 0, offset to padding = 0
>            bytes of callee saved registers = 0, exception handler addr = 0000:0000
>            local fp reg = RSP, param fp reg = RSP
>            flags = opt speed
>        0 | S_LDATA32 [size = 16] `A`
>            type = 0x0674 (int*), addr = 0000:0000
>        0 | S_INLINESITE [size = 24]
>            inlinee = 0x1003 (SUB), parent = 0, end = 0
>              0608      line 4 (+4)
>              033A      code 0x3A (+0x3A)
>              0406      code end 0x40 (+0x6)
>              0000
> 
> ```
> 
> 2. There is a separate S_GPROC32_ID emitted for IF_TEST::SUB. with its parent set to 0:
> 
> ```
>        0 | S_GPROC32_ID [size = 52] `IF_TEST::SUB`
>            parent = 0, end = 0, addr = 0000:0000, code size = 7
>            type = `0x1003 (SUB)`, debug start = 0, debug end = 0, flags = none
> ```
> 
> 1. looks fine but I am not sure about 2. I will look into this further. If you have any insight, please share with me.
> 
> Note, however, the entry in 2. is also emitted before the patch, so that would be a separate issue from the one I am addressing in this review.
> 
> 
I see, I think I understand what is going wrong and where my misunderstanding was. The issue is that getScopeIndex is inserting incorrect information into the TypeIndices table, which is later used for the main IF_TEST subprogram. That, understandably, leads to PDB linker errors.

However, as the reviewer, I'm expecting the change in behavior to appear in the 0x1003 record in your pasted dump:
> ```
> 0x1000 | LF_ARGLIST [size = 8]
> 0x1001 | LF_PROCEDURE [size = 16]
>          return type = 0x0003 (void), # args = 0, param list = 0x1000
>          calling conv = cdecl, options = None
> 0x1002 | LF_FUNC_ID [size = 20]
>          name = IF_TEST, type = 0x1001, parent scope = <no type>
> 0x1003 | LF_FUNC_ID [size = 16]
>          name = SUB, type = 0x1001, parent scope = 0x1002   #### parent scope is now 0x1002 instead of a string
> ```

What is the desired behavior for the `SUB` func id record? What should the parent scope be? I'm not aware of a way to make MSVC emit nested function id records like this, so we have no implementation to compare again. Should we create trees of LF_FUNC_ID records, or should we create a separate LF_STRING_ID record to use as the parent?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113142