<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Sep 25, 2020 at 5:20 PM Adrian Prantl <<a href="mailto:aprantl@apple.com">aprantl@apple.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="overflow-wrap: break-word;"><br><div><br><blockquote type="cite"><div>On Sep 25, 2020, at 4:36 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br><div><div>On Fri, Sep 25, 2020 at 4:08 PM Adrian Prantl <<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a>> wrote:<br><blockquote type="cite"><br>First — thanks for fixing the test for me!<br></blockquote><br>I'm a bit curious about the test - any idea how it came to be, if it's<br>invalid? Did we produce such bitcode in the past and don't anymore -<br>what's the rule about backwards compatibility here, then? It seems<br>like any time we regenerate a bitcode compat test that's questionable<br>because it means we won't be compatible with that bitcode we said we<br>should be compatible with?<br></div></div></blockquote><div><br></div><div>I think I mentioned in the commit message — we had a short period in clang with a bug</div><div><br></div><div><p style="margin:0px 0px 12px;padding:0px;border:0px">>>> Clang right before <a href="https://reviews.llvm.org/D79967" style="text-decoration:none;color:rgb(19,108,178);white-space:nowrap;margin-top:0px" target="_blank"><span style="color:rgb(70,76,92);border:1px solid rgb(207,219,227);border-radius:3px;padding:0px 4px;background-color:rgb(222,231,248);margin-top:0px"><span style="display:inline-block;color:rgb(107,116,140);line-height:1;margin:0px 4px 0px 2px"></span>https://reviews.llvm.org/D79967</span></a> would generate this kind of broken IR.</p><div>and the swift-5.3 branch happened to be cut in that unlucky window. To trigger it you need to markup a function with NoDebug (the bug was introduced  maybe a month earlier). </div></div></div></div></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="overflow-wrap: break-word;"><div> (snipped from other email): </div></div></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="overflow-wrap: break-word;"><div>I didn't answer the bitcode part: Adding the Verifier check *is* the bitcode compatibility. If an AssertDI fires, the debug info is stripped from the CU with a warning about invalid debug info. We don't need be implement a bitcode upgrade for a buggy version of clang, particularly not in this case, since I also cherry-picked the bugfix to that same branch.</div></div></blockquote><div> <br>Ah, OK, all makes sense. So invalid DWARF isn't invalid IR/rejected, it's just dropped (knew that on some level, but hadn't put it all together properly - appreciate you covering it), so some quirky IR in the past we can accept is buggy rather than just old and drop rather than handle.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="overflow-wrap: break-word;"><div><div><div><br></div></div><blockquote type="cite"><div><div><br><blockquote type="cite">We have two kinds of DISubprograms:<br>- 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.<br>- distinct DISubprograms describe one specific function definition. They belong to a compile unit and thus have a unit: field.<br><br>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.<br></blockquote><br>But do they have to be distinct? They have different fields (as you<br>say, declaration DISubprograms don't have a "unit:" field, definition<br>DISubprograms do have a unit: field that refers to a (itself distinct)<br>DICompileUnit)<br></div></div></blockquote><div><br></div><div>IIRC, DIBuilder lets you either create a distinct one with a unit or a uniqued one without.</div></div></div></blockquote><div><br></div><div>I'm with you on the implementation, but trying to understand the justification.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="overflow-wrap: break-word;"><div><div> I don't know if the backend would survive the same uniqued DISubprogram node being attached to multiple llvm::Functions or if that would invalidate some assumptions... In any case distinct seems to be a reasonable proxy for what we mean.</div></div></div></blockquote><div><br>Right, so maybe the clearer question would be: What would go wrong if DISubprogram wasn't distinct.<br><br>I don't think distinct stops a DISubprogram from being referred to from multiple llvm::Functions at the same time - it just stops the IR linker from deduplicating the DISubprograms when IR linking, I think? But since the DISubprograms refer to the DICompileUnit which is distinct, so the DISubprograms wouldn't be the same (they'd be referring to different DICompileUnits) so they wouldn't be deduplicated across IR modules when IR linking.<br><br>Perhaps marking them as distinct saves some link time by not requiring the IR Linker from /trying/ to merge them? So the generalization of that observation would be "any IR that refers to a distinct node can and should (but doesn't have to be) distinct itself to simplify/speed up IR linking, because it can't ever be deduplicated anyway"?<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="overflow-wrap: break-word;"><div><div><br></div><div>-- adrian</div><br><blockquote type="cite"><div><div><br><blockquote type="cite">We actually had a weaker form of the check I added to Verifier already in place (<a href="https://github.com/llvm/llvm-project/blob/51cad041e0cb26597c7ccc0fbfaa349b8fffbcda/llvm/lib/IR/Verifier.cpp#L1186" target="_blank">https://github.com/llvm/llvm-project/blob/51cad041e0cb26597c7ccc0fbfaa349b8fffbcda/llvm/lib/IR/Verifier.cpp#L1186</a>), but it didn't cover the specific testcase I added in <a href="https://reviews.llvm.org/rGe17f52d623cc146b7d9bf5a2e02965043508b4c4" target="_blank">https://reviews.llvm.org/rGe17f52d623cc146b7d9bf5a2e02965043508b4c4</a>.<br><br>-- adrian<br></blockquote></div></div></blockquote></div><br></div></blockquote></div></div>