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

[Nick Lewycky]

On 11 August 2014 16:36, Duncan P. N. Exon Smith <dexonsmith at apple.com>
wrote:

>
> > On 2014-Aug-11, at 16:00, Nick Lewycky <nlewycky at google.com> wrote:
> >
> > Sorry for the delay!
>
> Thanks for the review!  I've attached a new copy of the patch, with
> answers to your questions inline below.
>
> >> 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?
>
> IMO, writing to bitcode and reading back shouldn't change the IR.
>

It doesn't if you have current IR. Even llvm 1.x did IR
auto-upgrading. Sorry, but while what you're suggesting would indeed be a
nice property to have, it does not match llvm's established practice. If
you want to push for it then the right thing to do is bring a proposal up
on llvmdev. I know of one out-of-tree user who deals with us changing the
bitcode format by keeping multiple copies of lib/Bitcode around, one for
each version that their releases are based off of.

The motivating problem I'm solving is serializing use-list order to
> bitcode (PR5680).
>
> > 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.
>
> Does the plan in PR20506 sound reasonable?
>

Sounds reasonable to me, what it describes is an ABI break in slow motion
with notices and stuff. I'm not sure we have any ABI users who *actually
care* which is why I think it's better to move quickly (before we grow any
users who do care *and* start relying on the deprecated behaviour), but I'm
happy to let you want to handle this however you like.

> Also we should stop making such promises but that's a discussion in
> another thread.
>
> Agreed!
>
> >> 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?
>
> No, it's intentional :).  This `verify-uselistorder` invocation fails
> prior to the patch and passes after, which is how I came across the
> inconsistency.  I've tweaked the commit message to make that more clear.
>

Got it. I missed that this was about bitcode use list ordering work. Thanks
for working on that, by the way! Gnarly problem. Covered in barnacles.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140811/c7280ac0/attachment.html>