<div dir="ltr">This is the conclusion of everyone else as well, yes.<div><br></div><div>Please go ahead Tom, and thanks very much.</div><div><br></div><div>-eric</div></div><br><div class="gmail_quote"><div dir="ltr">On Tue, Nov 10, 2015 at 9:51 AM deadal nix <<a href="mailto:deadalnix@gmail.com">deadalnix@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><p dir="ltr">The thing is, 3.7 broke ABI not 3.7.1 . 3.7.1 is compatible with 3.6.x and 3.8.x (or what we know of it so far).</p>
<p dir="ltr">Reducing the window during which this ABI differs is a plus.</p>
<div class="gmail_quote">On Nov 10, 2015 09:13, "Tom Stellard" <<a href="mailto:tom@stellard.net" target="_blank">tom@stellard.net</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Mon, Nov 09, 2015 at 10:22:01PM +0000, Eric Christopher wrote:<br>
> We covered this particular patch a bit at the C API BoF at the conference,<br>
> and I think the general opinion is that this was just an unintentional bug<br>
> and that fixing it is in 3.7.1 is the best solution forward.<br>
><br>
<br>
Did anyone object to the ABI breakage this would cause between 3.7.0 and 3.7.1?<br>
My impression is that most users of the stable releases care a lot about ABI<br>
stability, and this change will affect everyone not just users of the C API.<br>
<br>
We will also need to update the SONAME for 3.7.1 and fix the symlinks to account<br>
for this change.<br>
<br>
If the consensus is that fixing the C API here is worth breaking ABI between<br>
3.7.0 and 3.7.1, that is fine. I just want to make sure people were aware<br>
of the downsides of breaking the ABI here.<br>
<br>
-Tom<br>
<br>
> I'll send out more on the C API BoF as I get out from under a pile of email.<br>
><br>
> -eric<br>
><br>
><br>
> On Mon, Nov 9, 2015 at 2:07 PM Reid Kleckner <<a href="mailto:rnk@google.com" target="_blank">rnk@google.com</a>> wrote:<br>
><br>
> > We knew this was an ABI break. Here was the logic for why we want it in<br>
> > the 3.7.1 release:<br>
> ><br>
> > The C API is supposed to be stable. The fact that the 3.7.0 release<br>
> > contained a modified LLVMBuildLandingPad function was a bug. Anyone who<br>
> > relies on this API basically cannot use the 3.7.0 release. The set of users<br>
> > using the C API and dealing with EH is probably small, so we chose not a to<br>
> > rush out a 3.7.1 release with a fix for this. Instead, we let the fix roll<br>
> > into 3.7.1, which will now have a backwards compatible C API and ABI with<br>
> > pre 3.7.0 LLVM.<br>
> ><br>
> > ---<br>
> ><br>
> > Anyway, I don't really care whether this gets merged or not. I am not a<br>
> > stakeholder in the stability of the C API. I would really prefer it if the<br>
> > C API users voiced an opinion on whether they want the C API to be more<br>
> > stable or closer to LLVM's in memory representation.<br>
> ><br>
> > On Mon, Nov 9, 2015 at 9:29 AM, Tom Stellard <<a href="mailto:tom@stellard.net" target="_blank">tom@stellard.net</a>> wrote:<br>
> ><br>
> >> On Wed, Oct 28, 2015 at 05:32:17AM +0000, Eric Christopher wrote:<br>
> >> > On Tue, Oct 27, 2015 at 8:15 PM Chris Lattner <<a href="mailto:sabre@nondot.org" target="_blank">sabre@nondot.org</a>> wrote:<br>
> >> ><br>
> >> > ><br>
> >> > > > On Oct 27, 2015, at 8:10 AM, Tom Stellard <<a href="mailto:tom@stellard.net" target="_blank">tom@stellard.net</a>> wrote:<br>
> >> > > ><br>
> >> > > > Hi Chris,<br>
> >> > > ><br>
> >> > > > I would like to get your opinion on merging r242372<br>
> >> > > > (<a href="http://reviews.llvm.org/rL242372" rel="noreferrer" target="_blank">http://reviews.llvm.org/rL242372</a>) into the 3.7 branch.<br>
> >> > > > The signature of the C API function LLVMBuildLandingPad<br>
> >> > > > changed from the 3.6.0 to 3.7.0 release, so the C API<br>
> >> > > > is currently incompatible between these to releases.<br>
> >> > ><br>
> >> > > This is a narrow enough API that it is probably only used by a few<br>
> >> > > clients.  I’d be happy for us to do whatever those clients would like<br>
> >> to<br>
> >> > > see with this.<br>
> >> > ><br>
> >> ><br>
> >> > Normally I'd prefer not to change API in a point release, but I think<br>
> >> it's<br>
> >> > fine to change it back here to match the previous releases and current<br>
> >> > trunk. Just needs something in the release notes.<br>
> >> ><br>
> >><br>
> >> Adding the correct llvm-dev list this time...<br>
> >><br>
> >> I've just realized that this patch changes the ABI and would make 3.7.0<br>
> >> and 3.7.1<br>
> >> binary incompatible.  I think breaking the stable ABI is worse than<br>
> >> breaking the C API,<br>
> >> so I would prefer to either drop this patch or come up with a work-around.<br>
> >><br>
> >> One possible work-around would be to keep the LLVMBuildLandingPad<br>
> >> signature the<br>
> >> same and then add this to Core.h:<br>
> >><br>
> >> #ifdef DEBUG // Not sure whether we should use this or ifndef NDEBUG<br>
> >>   #define LLVMBuildLandingPad(B, Ty, PersFn, Name) \<br>
> >>     dbgs() << "Warning: PersnFn parameter ignored.  You must explicitly<br>
> >> set the " \<br>
> >>               "personality function on the parent function with " \<br>
> >>               " LLVMSetPersonalityFn().  This behavior changed in LLVM<br>
> >> 3.7"; \<br>
> >>     LLVMBuildLandingPad(B, Ty, Name)<br>
> >> #else<br>
> >>   #define LLVMBuildLandingPad(B, Ty, PersFn, Name)<br>
> >>     LLVMBuildLandingPad(B, Ty, Name);<br>
> >><br>
> >> I'm open to other suggestions.  What do people think about this?<br>
> >><br>
> >> -Tom<br>
> >><br>
> >><br>
> >> ><br>
> >> ><br>
> >> > ><br>
> >> > > -Chris<br>
> >> > ><br>
> >> > > ><br>
> >> > > > Merging this commit will make the 3.7.1 C API compatible<br>
> >> > > > with 3.6.x and current trunk, but it will make the 3.7.1 C API<br>
> >> > > > incompatible with 3.7.0.<br>
> >> > > ><br>
> >> > > > Not merging this commit will mean the 3.7.x C API will<br>
> >> > > > be incompatible with 3.6.x and current trunk, but<br>
> >> > > > the C API for 3.7.0 and 3.7.1 will be compatible.<br>
> >> > > ><br>
> >> > > > It took me a while to wrap my head around this, let me know if you<br>
> >> have<br>
> >> > > > any questions.<br>
> >> > > ><br>
> >> > > > -Tom<br>
> >> > ><br>
> >> > ><br>
> >><br>
> ><br>
> ><br>
</blockquote></div>
</blockquote></div>