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

[Reid Kleckner]

On Mon, Aug 11, 2014 at 4:36 PM, 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.
>
> The motivating problem I'm solving is serializing use-list order to
> bitcode (PR5680).


The global_ctors auto-upgrade path is only one of many changes we make to
IR when deserializing it, though, so by this argument we shouldn't do any
auto-upgrade at all.


> > 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?


I think we still don't know what kind of high-level C API we would want
that would insulate us from these kinds of changes, and we're unlikely to
discover it soon.

If we wait a release cycle and document the change, I don't think anyone
will notice, and there will be just as much breakage in six months as there
would be now if we stared rejecting the 2-arg form.

I think we can do one of two things:
1. Keep the third argument as optional indefinitely, and drop the
auto-upgrade code.
2. Make it required starting in 3.5 and document a minor C API break. We
would keep the auto-upgrade code for old bitcode.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140811/484879c2/attachment.html>