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

Tue Nov 9 08:25:41 PST 2021

cchen15 added inline comments.

================
Comment at: llvm/test/DebugInfo/COFF/fortran-contained-proc.ll:25
+;
+; CHECK: S_GPROC32_ID [size = {{.*}}] `IF_TEST`
+; CHECK-NEXT: parent
----------------
rnk wrote:
> cchen15 wrote:
> > rnk wrote:
> > > 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?
> > > 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.
> > > 
> > Yes, exactly. My apologies. I should have done a better job explaining this. One oddity is that older linkers (say from VS2019 16.8.2 and earlier) would not issue the link error with IF_TEST having a LF_STRING_ID type.
> > 
> > > 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?
> > 
> > CodeVIew is very new to me, and I don't know what the proper way to describe the nested-ness of function scopes is. But, coming from DWARF, it makes sense to me to have the parent scope of 0x1003 pointing at 0x1002, reason being that a LD_FUNC_ID has a 'parent scope' field, which readily enables nesting of scopes. In contrast, LF_STRING_ID doesn't have a 'parent scope' (at least in the samples I have seen).
> > 
> > I can't find any document publicly on CodeView format. If you have any pointer, please share with me.  Thanks!
> I will forward this question to some contacts at Microsoft. In the meantime, I think it's safest to use the LF_STRING_ID record as the parent scope. This will make it look like the subroutine is in a C++ namespace, which is probably fine for the debugger.
> 
> Functionally, you can implement this by skipping the type table index insertion in getScopeIndex when the scope is a subprogram. Or you could change the key to differentiate it some other way, but I think skipping is simplest. There is another hash table at a lower level that deduplicates equivalent LF_STRING_ID records.
Thank you for your suggestion, Reid. I have updated the code to return zero index when encountering a DISubprogram (i.e. ekipping), and updated the test accordingly.

Thank you too for forwarding the question on encoding nested functions to MS. I appreciate it.

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

https://reviews.llvm.org/D113142