[PATCH] Don't upgrade global constructors when reading bitcode

[Nick Lewycky]

On 4 August 2014 22:43, Reid Kleckner <rnk at google.com> wrote:

> I was hoping Nick would throw in his two cents.  :)
>

Sorry for the delay!


> On Thu, Jul 31, 2014 at 2:26 PM, Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
>
>> An optional third field was added to `llvm.global_ctors` (and
>> `llvm.global_dtors`) in r209015.  Most of the code has been changed to
>> deal with both versions of the variables.  Users of the C API might
>> create either version, the helper functions in LLVM create the two-field
>> version, and clang now creates the three-field version.
>>
>
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.

However, the BitcodeReader was changed to always upgrade to the
>> three-field version.  This created an unnecessary inconsistency in the
>> IR before/after serializing to bitcode.
>>
>
What problems did this cause? What are you solving?

 This commit resolves the inconsistency by making the third field truly
>> optional (and not upgrading in the bitcode reader).  Since `llvm-link`
>> was relying on this upgrade code, rather than deleting it I've moved it
>> into `ModuleLinker`, where it upgrades these arrays as necessary to
>> resolve inconsistencies between modules.
>>
>> I changed `test/Bitcode/upgrade-global-ctors.ll` to a negative test and
>> duplicated the `llvm-link` check in `test/Linker/global_ctors.ll` to
>> check both upgrade directions.  Since I came across this as part of
>> PR5680 (serializing use-list order), I've also added a RUN line to
>> `test/Bitcode/metadata-2.ll`.
>>
>> The ideal resolution would be to remove the 2-field version and make the
>> third field required.  I filed PR20506 to track that.
>>
>
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:

  /* requires # ops space for "ops" */
  void FillFrom(LLVMValueRef Val, LLVMValueRef *ops) {
    for (int i = 0; i < LLVMGetNumOperands(Val); i++)
      ops[i] = LLVMGetOperand(Val, i);
  }

  [...]

    LLVMValueRef refs[2];
    FillOperands(my_global, &refs);

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.


+  assert(Initializers.size() == ATy->getNumElements());

Assert message please. (Yes I know it's just moved code.)

Most of the checking in upgradeMismatchedGlobalArray seems unnecessary, the
inputs must pass the verifier, right? For example:

+  auto *DstTy = dyn_cast<ArrayType>(DstGV->getType()->getElementType());
+  if (!DstTy)
+    return;
+  auto *SrcTy =
+      dyn_cast<ArrayType>(TypeMap.get(SrcGV->getType()->getElementType()));
+  if (!SrcTy)
+    return;

Those returns should be unreachable. If the "llvm.global_ctors" name
exists, it must have the right type.

+  auto *DstEltTy = dyn_cast<StructType>(DstTy->getElementType());
+  if (!DstEltTy)
+    return;

That return should also be unreachable.

diff --git a/test/Bitcode/metadata-2.ll b/test/Bitcode/metadata-2.ll
index 4055f92..06f8442 100644
--- a/test/Bitcode/metadata-2.ll
+++ b/test/Bitcode/metadata-2.ll
@@ -1,4 +1,5 @@
 ; RUN: llvm-as < %s | llvm-dis -disable-output
+; RUN: verify-uselistorder < %s -preserve-bc-use-list-order
  %0 = type { %object.ModuleInfo.__vtbl*, i8*, %"byte[]", %1,
%"ClassInfo[]", i32, void ()*, void ()*, void ()*, i8*, void ()* } ; type %0
  %1 = type { i64, %object.ModuleInfo* } ; type %1
  %2 = type { i32, void ()* } ; type %2

Did this creep in from the wrong patch?

Nick
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140811/ad6bd028/attachment.html>