Tom Stellard via llvm-dev
2015-Nov-09 17:29 UTC
[llvm-dev] Request to merge r242372 into the 3.7 branch: Fix for C API incompatibility between 3.6 and 3.7.
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 > > > >
Reid Kleckner via llvm-dev
2015-Nov-09 22:07 UTC
[llvm-dev] Request to merge r242372 into the 3.7 branch: Fix for C API incompatibility between 3.6 and 3.7.
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/53446d86/attachment.html>
Eric Christopher via llvm-dev
2015-Nov-09 22:22 UTC
[llvm-dev] 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>
Mehdi Amini via llvm-dev
2015-Nov-09 22:30 UTC
[llvm-dev] Request to merge r242372 into the 3.7 branch: Fix for C API incompatibility between 3.6 and 3.7.
> On Nov 9, 2015, at 2:07 PM, Reid Kleckner via llvm-dev <llvm-dev at lists.llvm.org> 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.During the BoF at the LLVM Dev meeting, if I understood correctly (I hope other can confirm, Eric?), some people asked about this issue and it was said that 3.7.1 will take the fix. — Mehdi> > On Mon, Nov 9, 2015 at 9:29 AM, Tom Stellard <tom at stellard.net <mailto: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 <mailto:sabre at nondot.org>> wrote: > > > > > > > > > On Oct 27, 2015, at 8:10 AM, Tom Stellard <tom at stellard.net <mailto:tom at stellard.net>> wrote: > > > > > > > > Hi Chris, > > > > > > > > I would like to get your opinion on merging r242372 > > > > (http://reviews.llvm.org/rL242372 <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 > > > > > > > > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20151109/7302548f/attachment.html>
Possibly Parallel Threads
- Request to merge r242372 into the 3.7 branch: Fix for C API incompatibility between 3.6 and 3.7.
- LLVMBuildLandingPad is wrong on 3.7
- 3.7.1-rc1 has been tagged. Let's begin testing!
- 3.7.1-rc1 has been tagged. Let's begin testing!
- [3.7.1 Release] -rc2 has been tagged