This is an archive of the discontinued LLVM Phabricator instance.

[CodeView] Properly handle a DISubprogram in getScopeIndex.
ClosedPublic

Authored by cchen15 on Nov 3 2021, 1:53 PM.

Details

Summary

There's no need to handle DISubprogram in getScopeIndex for C/C++ as a function can't be nested in another function. That's not the case for Fortran-- a routine can be nested in another routine through the 'contains' keyword. This patch skips the emission of the debug info for the containing function.

Please note that before this patch, getScopeIndex would emit a LF_STRING_ID when given a DISubprogram. That's not only incorrect, but would also cause link-time error with the linker from a more recent version (Aug. 20219) of VS2019.

Diff Detail

Event Timeline

cchen15 created this revision.Nov 3 2021, 1:53 PM
cchen15 requested review of this revision.Nov 3 2021, 1:53 PM
Herald added a project: Restricted Project. · View Herald Transcript
rnk added a comment.Nov 3 2021, 2:45 PM

Please note that before this patch, getScopeIndex would emit a LF_STRING_ID when given a DISubprogram. That's not only incorrect, but would also cause link-time error with the linker from a more recent version (Aug. 20219) of VS2019.

Really? It's what we do for C++ namespaces, though, right? I can't imagine why that would result in linker errors. Anyway, it's not important, I trust the new behavior is correct.

llvm/test/DebugInfo/COFF/fortran-contained-proc.ll
26

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?

cchen15 added a comment.EditedNov 4 2021, 7:00 AM

Thanks for the feedback, Reid!

Here's the linker error I am seeing with VS2019 16.11.2 before this patch is applied:

[cchen15@f90srv05 jr14335]$ llc fortran-contained-proc.ll -filetype=obj
[cchen15@f90srv05 jr14335]$ link -out:a.out -debug -pdb:a.pdb -subsystem:console fortran-contained-proc.obj
Microsoft (R) Incremental Linker Version 14.29.30133.0
Copyright (C) Microsoft Corporation.  All rights reserved.

libcmt.lib(commit_mode.obj) : fatal error LNK1318: Unexpected PDB error; OK (0) ''

My very uneducated guess is that the new versions of linker try to disallow a S_GPROC32_ID to have a type of LF_STRING_ID, but it's not done properly, resulting in this 'unexpected PDB error' as opposed to a more helpful diagnostic.

cchen15 added inline comments.Nov 4 2021, 7:30 AM
llvm/test/DebugInfo/COFF/fortran-contained-proc.ll
26

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

rnk added inline comments.Nov 4 2021, 10:28 AM
llvm/test/DebugInfo/COFF/fortran-contained-proc.ll
26

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?

cchen15 added inline comments.Nov 4 2021, 11:21 AM
llvm/test/DebugInfo/COFF/fortran-contained-proc.ll
26

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!

rnk added inline comments.Nov 8 2021, 1:21 PM
llvm/test/DebugInfo/COFF/fortran-contained-proc.ll
26

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.

cchen15 updated this revision to Diff 385835.Nov 9 2021, 8:23 AM
cchen15 edited the summary of this revision. (Show Details)
cchen15 added inline comments.Nov 9 2021, 8:25 AM
llvm/test/DebugInfo/COFF/fortran-contained-proc.ll
26

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.

rnk accepted this revision.Nov 9 2021, 10:02 AM

Looks good

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
366

My suggestion was to return TI here but skip recordTypeIndexForDINode when Scope is a subprogram, but this is fine too.

llvm/test/DebugInfo/COFF/fortran-contained-proc.ll
26

This test looks good, thanks.

This revision is now accepted and ready to land.Nov 9 2021, 10:02 AM
This revision was landed with ongoing or failed builds.Nov 9 2021, 10:18 AM
This revision was automatically updated to reflect the committed changes.

Thanks for the review, Reid.