[llvm] r200721 - DIBuilder: simplify array generation to produce true zero-length arrays

[David Blaikie]

On Mon, Feb 10, 2014 at 4:42 PM, Adrian Prantl <aprantl at apple.com> wrote:

>
> On Feb 10, 2014, at 16:14, David Blaikie <dblaikie at gmail.com> wrote:
>
> > +Bob & Adrian who particularly care about this
> >
> > I think the general consensus was to just bump the version number for
> each LLVM release (or private release - when any developer has one for
> which they need this).
> >
> > So I think we essentially want to flag the tree as "we've broken
> compatibility" and whoever hits a release first (internal or LLVM official)
> should bump the version at that point and then the tree is "unbroken".
> Rather than bumping it for every change.
> >
> If we do this we need to document the official procedure somewhere. We
> would need to have a Debug info canary somewhere as the flag that current
> trunk is incompatible to any previously released version although the
> version number is the same and then anyone who cares may reset the flag and
> bump the version number (at any time?).
>
> We really need to make sure that it will not be easy to accidentally
> release two incompatible incarnations of LLVM with the same Debug info
> version number.
>
> > We don't want to do this more often than necessary because it involves
> updating /all/ the debug info test cases.
> >
>
> We should also provide a script that recursively greps for
>   ![[0-9]]+ = metadata !{i32 2, metadata !"Dwarf Version", i32 [[0-9]]+}
> and bump the version. I guess it could also reset the canary.
>

Yes, providing a script that does this seems like a fairly simple/good idea
- it's really just a one-liner of find + sed (plus another line to reset
the canary, as you say).


> Now that I think about it, I believe it would be better to provide only
> this script and run it every time someone breaks compatibility.


That's going to be pretty churny - granted we don't break things often at
the moment, but this does seem a bit painful, especially if you propose
doing this in the same commit as the version break (because then code
review becomes harder - finding the 2 test cases with meaningful changes
amongst a field of version bumps?)


> Why? Suppose we have two changes on trunk that break compatibility: Change
> 1 sets the flag. Change 2 later doesn't, because it's already there, and
> there was no release in the meantime. When someone needs to cherry-pick
> only Change 2 into a release branch, because it fixes an important bug,
> there is no way to tell whether that branch should bump the version number
> or not.


Not sure there's a great answer here, but we could have some means to touch
the canary file each time debug info version is broken (even if it's
already in a "broken" state). I'm not sure if you'd have trouble with this
if you cherry picked Change 2, bumped the upstream version, cherry picked
that, then later on integrated the whole branch - not realizing you needed
another bump.

I think I see the problem you're dealing with now - if you can pick up
arbitrary revisions you made need arbitrarily many version bumps and simply
having a version bump after all breaks may not be sufficient for your needs.

I think this is a bit unfortunate - but not sure just how much it matters.
Perhaps we can have a blessed "magic/any version" which we use for test
cases, and just one test case to check the version handling?


> (Obviously version numbers are only meaningful within the same "family" of
> releases.)
>
> base (V 100)
>  |
> change 1 (V101)
>  |
> change 2 (V102)
>  |
> ToT
>
> For private releases, one could use an expanded namespace:
> my_release1(base) (V 100100)
> my_release1.1(base+change2) (V 100101)
>
> my_release2(ToT) (V 200102)
>
>
>
> -- adrian
> >
> > On Mon, Feb 10, 2014 at 2:26 PM, Eric Christopher <echristo at gmail.com>
> wrote:
> > Might want to increase the debug version number because of this.
> > Previously working code will start asserting/crashing (see PR18790).
> >
> > -eric
> >
> > On Mon, Feb 3, 2014 at 3:08 PM, David Blaikie <dblaikie at gmail.com>
> wrote:
> > > Author: dblaikie
> > > Date: Mon Feb  3 17:08:54 2014
> > > New Revision: 200721
> > >
> > > URL: http://llvm.org/viewvc/llvm-project?rev=200721&view=rev
> > > Log:
> > > DIBuilder: simplify array generation to produce true zero-length arrays
> > >
> > > For some anachronistic reason we were producing {i32 0} for zero-length
> > > debug info arrays.
> > >
> > > (this change is paired with a Clang change and may cause temporary
> > > buildbot noise)
> > >
> > > Let's not.
> > >
> > > Modified:
> > >     llvm/trunk/lib/IR/DIBuilder.cpp
> > >
> > > Modified: llvm/trunk/lib/IR/DIBuilder.cpp
> > > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/DIBuilder.cpp?rev=200721&r1=200720&r2=200721&view=diff
> > >
> ==============================================================================
> > > --- llvm/trunk/lib/IR/DIBuilder.cpp (original)
> > > +++ llvm/trunk/lib/IR/DIBuilder.cpp Mon Feb  3 17:08:54 2014
> > > @@ -902,10 +902,6 @@ DIBuilder::createForwardDecl(unsigned Ta
> > >
> > >  /// getOrCreateArray - Get a DIArray, create one if required.
> > >  DIArray DIBuilder::getOrCreateArray(ArrayRef<Value *> Elements) {
> > > -  if (Elements.empty()) {
> > > -    Value *Null = Constant::getNullValue(Type::getInt32Ty(VMContext));
> > > -    return DIArray(MDNode::get(VMContext, Null));
> > > -  }
> > >    return DIArray(MDNode::get(VMContext, Elements));
> > >  }
> > >
> > >
> > >
> > > _______________________________________________
> > > llvm-commits mailing list
> > > llvm-commits at cs.uiuc.edu
> > > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140210/21ed7b60/attachment.html>