Fangrui Song via llvm-dev
2020-Sep-25 22:32 UTC
[llvm-dev] Why does a DISubprogram need to be distinct?
I saw https://reviews.llvm.org/rGe17f52d623cc146b7d9bf5a2e02965043508b4c4#949392 "Add a verifier check that rejects non-distinct DISubprogram function" (which has currently be reverted) today which made me wonder why a DISubprogram needs to be distinct? David told me the following and encouraged me to ask in the upstream:> May be historical at this point, I'm not quite sure. (I remember there being some complicated issues around distinctness of nodes when it came to inlining that could cause two inlined functions to end up smooshed together and appear as one inlined function - but I think that was about the distinctness of the debugloc itself, not of the function). > > Hmm, maybe it's from the days when DISubprogram ownership was inverted - DISubprograms used to be owned by the DICompileUnit and the DISubprogram would refer to the llvm::Function rather than the way it is now (llvm::Function refers to DISubprogram, DISubprogram refers to DICOmpileUnit). In that old way, deduplicating a DISubprogram when IR linking would have resulted in two DICompileUnits both having the DISubprogram in their subprogram list - which would break a bunch of other stuff. > > So /maybe/ not needed anymore? (given that the DISubprogram refers to the DICompileUnit which is distinct I can't see how the DISubprogram could have problems being non-distinct - but I might be missing something)I re-generated DISubprogram-v4.ll.bc with a llc 7.0.0 in 7d0556fc137aa07347741b7750e50ecbc2b4c6e2 to fix the built bots but I am not sure I did the right thing. * If old clang did not produce non-distinct DISubprogram, then even with the verifier I think the bitcode compatibility is still retained. * If old clang did produce distinct DISubprogram, this seems like a bitcode compatibility break due to a verifier?
Adrian Prantl via llvm-dev
2020-Sep-25 23:07 UTC
[llvm-dev] Why does a DISubprogram need to be distinct?
First — thanks for fixing the test for me! We have two kinds of DISubprograms: - uniqued DISubprograms are part of the type system and describe function *types*. They don't have a unit: field, because they want to be uniqued across compile units. - distinct DISubprograms describe one specific function definition. They belong to a compile unit and thus have a unit: field. You are only supposed to attach distinct DISubprograms to function definitions. Several places in CodeGen will unconditionally dereference the unit: field and crash if it isn't there. We actually had a weaker form of the check I added to Verifier already in place (https://github.com/llvm/llvm-project/blob/51cad041e0cb26597c7ccc0fbfaa349b8fffbcda/llvm/lib/IR/Verifier.cpp#L1186), but it didn't cover the specific testcase I added in https://reviews.llvm.org/rGe17f52d623cc146b7d9bf5a2e02965043508b4c4. -- adrian
David Blaikie via llvm-dev
2020-Sep-25 23:36 UTC
[llvm-dev] Why does a DISubprogram need to be distinct?
On Fri, Sep 25, 2020 at 4:08 PM Adrian Prantl <aprantl at apple.com> wrote:> > First — thanks for fixing the test for me!I'm a bit curious about the test - any idea how it came to be, if it's invalid? Did we produce such bitcode in the past and don't anymore - what's the rule about backwards compatibility here, then? It seems like any time we regenerate a bitcode compat test that's questionable because it means we won't be compatible with that bitcode we said we should be compatible with?> We have two kinds of DISubprograms: > - uniqued DISubprograms are part of the type system and describe function *types*. They don't have a unit: field, because they want to be uniqued across compile units. > - distinct DISubprograms describe one specific function definition. They belong to a compile unit and thus have a unit: field. > > You are only supposed to attach distinct DISubprograms to function definitions. Several places in CodeGen will unconditionally dereference the unit: field and crash if it isn't there.But do they have to be distinct? They have different fields (as you say, declaration DISubprograms don't have a "unit:" field, definition DISubprograms do have a unit: field that refers to a (itself distinct) DICompileUnit)> We actually had a weaker form of the check I added to Verifier already in place (https://github.com/llvm/llvm-project/blob/51cad041e0cb26597c7ccc0fbfaa349b8fffbcda/llvm/lib/IR/Verifier.cpp#L1186), but it didn't cover the specific testcase I added in https://reviews.llvm.org/rGe17f52d623cc146b7d9bf5a2e02965043508b4c4. > > -- adrian