llvm dev - Nov 2015 - Request to merge r242372 into the 3.7 branch: Fix for C API incompatibility between 3.6 and 3.7.

We covered this particular patch a bit at the C API BoF at the conference,
and I think the general opinion is that this was just an unintentional bug
and that fixing it is in 3.7.1 is the best solution forward.

I'll send out more on the C API BoF as I get out from under a pile of email.

-eric


On Mon, Nov 9, 2015 at 2:07 PM Reid Kleckner <rnk at google.com> wrote:
> We knew this was an ABI break. Here was the logic for why we want it in
> the 3.7.1 release:
>
> The C API is supposed to be stable. The fact that the 3.7.0 release
> contained a modified LLVMBuildLandingPad function was a bug. Anyone who
> relies on this API basically cannot use the 3.7.0 release. The set of users
> using the C API and dealing with EH is probably small, so we chose not a to
> rush out a 3.7.1 release with a fix for this. Instead, we let the fix roll
> into 3.7.1, which will now have a backwards compatible C API and ABI with
> pre 3.7.0 LLVM.
>
> ---
>
> Anyway, I don't really care whether this gets merged or not. I am not a
> stakeholder in the stability of the C API. I would really prefer it if the
> C API users voiced an opinion on whether they want the C API to be more
> stable or closer to LLVM's in memory representation.
>
> On Mon, Nov 9, 2015 at 9:29 AM, Tom Stellard <tom at stellard.net>
wrote:
>
>> On Wed, Oct 28, 2015 at 05:32:17AM +0000, Eric Christopher wrote:
>> > On Tue, Oct 27, 2015 at 8:15 PM Chris Lattner <sabre at
nondot.org> wrote:
>> >
>> > >
>> > > > On Oct 27, 2015, at 8:10 AM, Tom Stellard <tom at
stellard.net> wrote:
>> > > >
>> > > > Hi Chris,
>> > > >
>> > > > I would like to get your opinion on merging r242372
>> > > > (http://reviews.llvm.org/rL242372) into the 3.7 branch.
>> > > > The signature of the C API function LLVMBuildLandingPad
>> > > > changed from the 3.6.0 to 3.7.0 release, so the C API
>> > > > is currently incompatible between these to releases.
>> > >
>> > > This is a narrow enough API that it is probably only used by
a few
>> > > clients.  I’d be happy for us to do whatever those clients
would like
>> to
>> > > see with this.
>> > >
>> >
>> > Normally I'd prefer not to change API in a point release, but
I think
>> it's
>> > fine to change it back here to match the previous releases and
current
>> > trunk. Just needs something in the release notes.
>> >
>>
>> Adding the correct llvm-dev list this time...
>>
>> I've just realized that this patch changes the ABI and would make
3.7.0
>> and 3.7.1
>> binary incompatible.  I think breaking the stable ABI is worse than
>> breaking the C API,
>> so I would prefer to either drop this patch or come up with a
work-around.
>>
>> One possible work-around would be to keep the LLVMBuildLandingPad
>> signature the
>> same and then add this to Core.h:
>>
>> #ifdef DEBUG // Not sure whether we should use this or ifndef NDEBUG
>>   #define LLVMBuildLandingPad(B, Ty, PersFn, Name) \
>>     dbgs() << "Warning: PersnFn parameter ignored.  You must
explicitly
>> set the " \
>>               "personality function on the parent function with
" \
>>               " LLVMSetPersonalityFn().  This behavior changed in
LLVM
>> 3.7"; \
>>     LLVMBuildLandingPad(B, Ty, Name)
>> #else
>>   #define LLVMBuildLandingPad(B, Ty, PersFn, Name)
>>     LLVMBuildLandingPad(B, Ty, Name);
>>
>> I'm open to other suggestions.  What do people think about this?
>>
>> -Tom
>>
>>
>> >
>> >
>> > >
>> > > -Chris
>> > >
>> > > >
>> > > > Merging this commit will make the 3.7.1 C API compatible
>> > > > with 3.6.x and current trunk, but it will make the 3.7.1
C API
>> > > > incompatible with 3.7.0.
>> > > >
>> > > > Not merging this commit will mean the 3.7.x C API will
>> > > > be incompatible with 3.6.x and current trunk, but
>> > > > the C API for 3.7.0 and 3.7.1 will be compatible.
>> > > >
>> > > > It took me a while to wrap my head around this, let me
know if you
>> have
>> > > > any questions.
>> > > >
>> > > > -Tom
>> > >
>> > >
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20151109/32a407d2/attachment.html>

On Mon, Nov 09, 2015 at 10:22:01PM +0000, Eric Christopher
wrote:
> We covered this particular patch a bit at the C API BoF at the conference,
> and I think the general opinion is that this was just an unintentional bug
> and that fixing it is in 3.7.1 is the best solution forward.
> 
Did anyone object to the ABI breakage this would cause between 3.7.0 and 3.7.1?
My impression is that most users of the stable releases care a lot about ABI
stability, and this change will affect everyone not just users of the C API.

We will also need to update the SONAME for 3.7.1 and fix the symlinks to account
for this change.

If the consensus is that fixing the C API here is worth breaking ABI between
3.7.0 and 3.7.1, that is fine. I just want to make sure people were aware
of the downsides of breaking the ABI here.

-Tom
> I'll send out more on the C API BoF as I get out from under a pile of
email.
> 
> -eric
> 
> 
> On Mon, Nov 9, 2015 at 2:07 PM Reid Kleckner <rnk at google.com>
wrote:
> 
> > We knew this was an ABI break. Here was the logic for why we want it
in
> > the 3.7.1 release:
> >
> > The C API is supposed to be stable. The fact that the 3.7.0 release
> > contained a modified LLVMBuildLandingPad function was a bug. Anyone
who
> > relies on this API basically cannot use the 3.7.0 release. The set of
users
> > using the C API and dealing with EH is probably small, so we chose not
a to
> > rush out a 3.7.1 release with a fix for this. Instead, we let the fix
roll
> > into 3.7.1, which will now have a backwards compatible C API and ABI
with
> > pre 3.7.0 LLVM.
> >
> > ---
> >
> > Anyway, I don't really care whether this gets merged or not. I am
not a
> > stakeholder in the stability of the C API. I would really prefer it if
the
> > C API users voiced an opinion on whether they want the C API to be
more
> > stable or closer to LLVM's in memory representation.
> >
> > On Mon, Nov 9, 2015 at 9:29 AM, Tom Stellard <tom at
stellard.net> wrote:
> >
> >> On Wed, Oct 28, 2015 at 05:32:17AM +0000, Eric Christopher wrote:
> >> > On Tue, Oct 27, 2015 at 8:15 PM Chris Lattner <sabre at
nondot.org> wrote:
> >> >
> >> > >
> >> > > > On Oct 27, 2015, at 8:10 AM, Tom Stellard <tom
at stellard.net> wrote:
> >> > > >
> >> > > > Hi Chris,
> >> > > >
> >> > > > I would like to get your opinion on merging r242372
> >> > > > (http://reviews.llvm.org/rL242372) into the 3.7
branch.
> >> > > > The signature of the C API function
LLVMBuildLandingPad
> >> > > > changed from the 3.6.0 to 3.7.0 release, so the C
API
> >> > > > is currently incompatible between these to
releases.
> >> > >
> >> > > This is a narrow enough API that it is probably only
used by a few
> >> > > clients.  I’d be happy for us to do whatever those
clients would like
> >> to
> >> > > see with this.
> >> > >
> >> >
> >> > Normally I'd prefer not to change API in a point release,
but I think
> >> it's
> >> > fine to change it back here to match the previous releases
and current
> >> > trunk. Just needs something in the release notes.
> >> >
> >>
> >> Adding the correct llvm-dev list this time...
> >>
> >> I've just realized that this patch changes the ABI and would
make 3.7.0
> >> and 3.7.1
> >> binary incompatible.  I think breaking the stable ABI is worse
than
> >> breaking the C API,
> >> so I would prefer to either drop this patch or come up with a
work-around.
> >>
> >> One possible work-around would be to keep the LLVMBuildLandingPad
> >> signature the
> >> same and then add this to Core.h:
> >>
> >> #ifdef DEBUG // Not sure whether we should use this or ifndef
NDEBUG
> >>   #define LLVMBuildLandingPad(B, Ty, PersFn, Name) \
> >>     dbgs() << "Warning: PersnFn parameter ignored.  You
must explicitly
> >> set the " \
> >>               "personality function on the parent function
with " \
> >>               " LLVMSetPersonalityFn().  This behavior
changed in LLVM
> >> 3.7"; \
> >>     LLVMBuildLandingPad(B, Ty, Name)
> >> #else
> >>   #define LLVMBuildLandingPad(B, Ty, PersFn, Name)
> >>     LLVMBuildLandingPad(B, Ty, Name);
> >>
> >> I'm open to other suggestions.  What do people think about
this?
> >>
> >> -Tom
> >>
> >>
> >> >
> >> >
> >> > >
> >> > > -Chris
> >> > >
> >> > > >
> >> > > > Merging this commit will make the 3.7.1 C API
compatible
> >> > > > with 3.6.x and current trunk, but it will make the
3.7.1 C API
> >> > > > incompatible with 3.7.0.
> >> > > >
> >> > > > Not merging this commit will mean the 3.7.x C API
will
> >> > > > be incompatible with 3.6.x and current trunk, but
> >> > > > the C API for 3.7.0 and 3.7.1 will be compatible.
> >> > > >
> >> > > > It took me a while to wrap my head around this, let
me know if you
> >> have
> >> > > > any questions.
> >> > > >
> >> > > > -Tom
> >> > >
> >> > >
> >>
> >
> >

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).

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