bn_mul.h: fix x86 PIC inline ASM compilation with GCC < 5

[jacmet]

Fixes #1910

With ebx added to the MULADDC_STOP clobber list to fix #1550, the inline
assembly fails to build with GCC < 5 in PIC mode with the following error:

include/mbedtls/bn_mul.h:46:13: error: PIC register clobbered by ‘ebx’ in ‘asm’

This is because older GCC versions treated the x86 ebx register (which is
used for the GOT) as a fixed reserved register when building as PIC.

This is fixed by an improved register allocator in GCC 5+. From the release
notes:

Register allocation improvements: Reuse of the PIC hard register, instead of
using a fixed register, was implemented on x86/x86-64 targets. This
improves generated PIC code performance as more hard registers can be used.

https://www.gnu.org/software/gcc/gcc-5/changes.html

As a workaround, detect this situation and disable the inline assembly,
similar to the MULADDC_CANNOT_USE_R7 logic.

Signed-off-by: Peter Korsgaard peter@korsgaard.com

Status

READY

Requires Backporting

Yes
Backport to 2.28 is #6096

Todos

• Tests
• Documentation
• Changelog updated
• Backported

Steps to test or reproduce

Outline the steps to test or reproduce the PR here.

[jacmet]

Example of failing build: http://autobuild.buildroot.net/results/ab5/ab5a58ea7845f9f378454ee1aa7e872448618ba9/build-end.log

[RonEld]

@jacmet Thank you for your contribution!
Out of curiosity, do you think it's reasonable to support old GCC versions in our development branch?
I believe our LTS branches should probably have such a fix, but I'm not sure our development branch should.

In addition, unfortunately our policy is to not accept contributions, without a Contributor’s Licence Agreement (CLA) signed or authorised by yourself or your employer.

If this is a personal contribution, the easiest way to do this is if you create an mbed account and accept this click through agreement. Alternatively, you can find a slightly different agreement to sign here, which can be signed and returned to us, and is applicable if you don't want to create an mbed account or alternatively if this is a corporate contribution.

Thanks for your understanding and again, thanks for the contribution!

[jacmet]

@RonEld It may not make sense to support it for new development, that is up to you. On the other hand, it doesn't add a lot of overhead. For the LTS branch it IMHO certainly makes sense (E.G. this triggered by the security bump to 2.7.5)

I already accepted the CLA, but didn't have my email confirmed (done now).

[RonEld]

@jacmet Thank you for your confirmation, and for accepting the CLA.
Could you confirm that this is you Mbed account?
Unfortunately, since github accounts and Mbed accounts are not linked together, we need to manually search the accounts by user names.
If this is your account, could you kindly make it public, so we could verify the CLA?

[jacmet]

@RonEld correct, and I have now made the account info public

[RonEld]

Thanks again for accepting the CLA!

[wyattoday]

Just putting in my 2-cents: I believe this patch should be applied to the development branch, because otherwise we would need to make this change manually on the older platforms we support. I know this is working around a bug in an old gcc, and that it's not necessarily in a project's best interest to support every compiler version under the sun, however I thought I would let you know in case you were wondering whether "real people" needed this change. We do. And without this fix merged to development we will need to make this change manually when upgrading to newer versions of mbedTLS (like we did with mbedTLS 2.12.0)

Again, this isn't a showstopper for us, but it does add extra work when upgrading mbedTLS.

[RonEld]

@wyattoday Thank you for informing us your use case. We will take that into consideration.
Is updating your gcc version for your older platforms as well an option for you?

[jacmet]

@RonEld I see travis fails on gcc, but don't quite understand the relation to the change (unless something is broken with the non-asm version now used for gcc < 5). Any idea?

[wyattoday]

@RonEld

Is updating your gcc version for your older platforms as well an option for you?

Unfortunately not. Not yet, at least. And not on some platforms. For example we release binaries that work "out of the box" on CentOS 5 and CentOS 6 (Redhat 5 / 6), which means we need to use the built-in glibc on the system (and thus use the built-in gcc on the system). Both CentOS 5 and CentOS 6 (RH 5 / 6) use gcc versions older than gcc 5.

CentOS 5 is admittedly old (and RH 5 only has 2 years and 2 months of "Extended life-cycle support" left). But CentOS 6 is still widely used by our customers and Redhat still supports RH 6 in the "Maintenance Support 2" cycle for the next 2 years 2 months (and Extended life-cycle of RH 6 is for the next 6 years).

[RonEld]

@wyattoday Thank you for your detailed use case. It sounds like a legitimate use case. WE will consider it.

@jacmet I tried reproducing this failure on my machine, using gcc 4.8 , and I didn't encounter the failure. Of course, there could be other environmental differences between my machine and Travis CI which I overlooked.
At the moment, I believe it might be some timing issue.

[tsl0922]

Thank you! This patch helped me to build mbedtls for openwrt x86 based on the 14.07 SDK with gcc-4.8.

[mpg]

Thank you for your contribution, and sorry for the very long period of inactivity on our side. We're going over our backlog of PRs, and were wondering if this is still relevant to you? If it is, can you rebase your PR on top of recent development, and we'll be sure to review quickly. Otherwise, this can be closed as the relevance of GCC <5 will only decline over time.

[jacmet]

@mpg We're still carrying this patch in Buildroot, so it is still relevant for us: https://git.buildroot.net/buildroot/tree/package/mbedtls/0001-bn_mul.h-fix-x86-PIC-inline-ASM-compilation-with-GCC.patch

[wyattoday]

@mpg This no longer impacts us. We've dropped support for unsupported platforms and compilers. We now use CentOS 7 with the latest devtoolset (which at the time of writing is gcc 11). This lets us target ancient glibc while using modern compilers.

[jacmet]

@mpg Buildroot so far still supports GCC 4.x (host/target)

[wyattoday]

@jacmet

Even the Linux kernel has moved away from gcc 4.x (recently requiring 5.1 and newer to build it).

Just FYI in case you were hanging on to ancient versions for compatibility reasons.

[mpg]

@jacmet Thanks for letting us know. This PR has a conflict, can you rebase in order to resolve it? If you do so, we'll be sure to review quickly so that this can finally be merged. (As a bugfix, it will also need a backport to the mbedtls-2.28 branch.)

[mckaygerhard]

I forgot about this, I was too busy, this patch must be apply .. yet many devices still use gcc 4.X for example embedded... this patch should still be supported because linux kernels like 4.X are still in use... for devices that can't use 5.X as an example, i cannot rebase, cos i used older devices (have a lot of those with large life and i will not change cos represent a cost)

i will try next weekend if there's no responses here for #1910

[tom-cosgrove-arm]

@jacmet Are you able to rebase this PR if it is still wanted? Or do you mind if someone else does it?

[tom-cosgrove-arm]

I have rebased this PR on top of latest development, and checked that (1) without it, i386 gcc 4 still gives the error and (2) with it, i386 gcc 4 no longer gives the error

[mpg]

The fix looks correct to me (at least in the sense that it's reasonable and should not affect other platforms). As discussed elsewhere, probably needs a ChangeLog entry and a backport, considering it's a bugfix.

[mpg]

LGTM