<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 4 August 2014 22:43, Reid Kleckner <span dir="ltr"><<a href="mailto:rnk@google.com" target="_blank">rnk@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr">I was hoping Nick would throw in his two cents. :)</div></blockquote><div><br></div><div>Sorry for the delay!</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class=""><div class="h5"><div class="gmail_extra"><div class="gmail_quote">On Thu, Jul 31, 2014 at 2:26 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">An optional third field was added to `llvm.global_ctors` (and<br>
`llvm.global_dtors`) in r209015. Most of the code has been changed to<br>
deal with both versions of the variables. Users of the C API might<br>
create either version, the helper functions in LLVM create the two-field<br>
version, and clang now creates the three-field version.<br></blockquote></div></div></div></div></blockquote><div><br></div><div>Users of the C API might require the 2-argument form and crash when not receiving the version they expect, thanks to direct access like LLVMGetNamedGlobal + LLVMGetNumOperands. Anything that is a verifier guarantee is also an ABI guarantee.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class=""><div class="h5"><div class="gmail_extra">
<div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">However, the BitcodeReader was changed to always upgrade to the<br>
three-field version. This created an unnecessary inconsistency in the<br>
IR before/after serializing to bitcode.<br></blockquote></div></div></div></div></blockquote><div><br></div><div>What problems did this cause? What are you solving?</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class=""><div class="h5"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
This commit resolves the inconsistency by making the third field truly<br>
optional (and not upgrading in the bitcode reader). Since `llvm-link`<br>
was relying on this upgrade code, rather than deleting it I've moved it<br>
into `ModuleLinker`, where it upgrades these arrays as necessary to<br>
resolve inconsistencies between modules.<br>
<br>
I changed `test/Bitcode/upgrade-global-ctors.ll` to a negative test and<br>
duplicated the `llvm-link` check in `test/Linker/global_ctors.ll` to<br>
check both upgrade directions. Since I came across this as part of<br>
PR5680 (serializing use-list order), I've also added a RUN line to<br>
`test/Bitcode/metadata-2.ll`.<br>
<br>
The ideal resolution would be to remove the 2-field version and make the<br>
third field required. I filed PR20506 to track that.<br></blockquote></div></div></div></div></blockquote><div><br></div><div>I'll assume there's a good reason we aren't doing the ideal thing now. We should update to make 3 required and if anyone was using the C API and if anyone's doing something like:</div>
<div><br></div><div><div> /* requires # ops space for "ops" */</div><div> void FillFrom(LLVMValueRef Val, LLVMValueRef *ops) {<br></div><div> for (int i = 0; i < LLVMGetNumOperands(Val); i++)</div><div>
ops[i] = LLVMGetOperand(Val, i);</div>
<div> }</div></div><div><br></div><div><div> [...]</div></div><div><br></div><div> LLVMValueRef refs[2];<br></div><div> FillOperands(my_global, &refs);</div><div><br></div><div>then they get to notice that we broke our promises. Also we should stop making such promises but that's a discussion in another thread.</div>
<div><br></div><div><br></div><div>+ assert(Initializers.size() == ATy->getNumElements());<br></div><div><br></div><div>Assert message please. (Yes I know it's just moved code.)</div><div><br></div><div>Most of the checking in upgradeMismatchedGlobalArray seems unnecessary, the inputs must pass the verifier, right? For example:</div>
<div><div><br></div><div><div>+ auto *DstTy = dyn_cast<ArrayType>(DstGV->getType()->getElementType());</div><div>+ if (!DstTy)</div><div>+ return;</div><div>+ auto *SrcTy =</div><div>+ dyn_cast<ArrayType>(TypeMap.get(SrcGV->getType()->getElementType()));</div>
<div>+ if (!SrcTy)</div><div>+ return;</div></div><div><br></div><div>Those returns should be unreachable. If the "llvm.global_ctors" name exists, it must have the right type.</div><div><br></div><div>+ auto *DstEltTy = dyn_cast<StructType>(DstTy->getElementType());</div>
<div>+ if (!DstEltTy)</div><div>+ return;</div></div><div><br></div><div>That return should also be unreachable.</div><div><br></div><div><div>diff --git a/test/Bitcode/metadata-2.ll b/test/Bitcode/metadata-2.ll</div>
<div>index 4055f92..06f8442 100644</div><div>--- a/test/Bitcode/metadata-2.ll</div><div>+++ b/test/Bitcode/metadata-2.ll</div><div>@@ -1,4 +1,5 @@</div><div> ; RUN: llvm-as < %s | llvm-dis -disable-output</div><div>+; RUN: verify-uselistorder < %s -preserve-bc-use-list-order</div>
<div> <span class="" style="white-space:pre"> </span>%0 = type { %object.ModuleInfo.__vtbl*, i8*, %"byte[]", %1, %"ClassInfo[]", i32, void ()*, void ()*, void ()*, i8*, void ()* }<span class="" style="white-space:pre"> </span>; type %0</div>
<div> <span class="" style="white-space:pre"> </span>%1 = type { i64, %object.ModuleInfo* }<span class="" style="white-space:pre"> </span>; type %1</div><div> <span class="" style="white-space:pre"> </span>%2 = type { i32, void ()* }<span class="" style="white-space:pre"> </span>; type %2</div>
</div><div><br></div><div>Did this creep in from the wrong patch?</div><div><br></div><div>Nick<br></div></div></div></div>