Expand i386 all.sh tests to include full configuration ASan builds #1810

simonbutcher · 2018-06-28T06:48:02Z

Description

The i386 test builds were only building the default configuration and had no address sanitisation. This pull request expands the test configuration to the full configuration in all.sh and generates ASan builds for test suite execution.

This pull request extends test coverage sufficiently that issue #1550, where the MPI assembly code on i386 which was fundamentally broken, would have been caught.

It also fixes the inline assembly which was previously only being included in builds with no optimisation, which was the opposite of what it should have been doing.

Once reviewed, this PR needs backporting.

Status

READY

Requires Backporting

Yes

Which branch?
mbedtls-2.7 and mbedtls-2.1

Todos

Tests
Documentation
Changelog updated
Backported

RonEld · 2018-06-28T07:37:54Z

tests/scripts/all.sh

    make test

    msg "build: 64-bit ILP32, make, gcc" # ~ 30s
    cleanup
+    cp "$CONFIG_H" "$CONFIG_BAK"
+    scripts/config.pl full
    make CC=gcc CFLAGS='-Werror -Wall -Wextra -mx32'


wouldn't we want ASan build for ILP32 as well?

Installing libasan for x32 didn't seem straight forward, and I guess isn't commonly used.

If you feel we should have it, I suggest we raise a bug to avoid holding up this PR, as we'd likely have to install it outside of the package manager to get it to work.

RonEld

The changes look good.
I just added a question I would like answered before I approve

RonEld · 2018-06-28T08:06:13Z

Isn't this dependant on #1778 to be merged first?

mpg

The code changes look good to me, however I'm having trouble testing it.

If I try to run all.sh in this branch, it fails as expected. So far so good. Then I created a temporary branch and merged #1778 into this one before trying again, only to run into a new error:

bignum.c: In function ‘mpi_mul_hlp’:
../include/mbedtls/bn_mul.h:46:13: error: ‘asm’ operand has impossible constraints
 #define asm __asm
             ^
../include/mbedtls/bn_mul.h:55:5: note: in expansion of macro ‘asm’
     asm(                                    \
     ^
bignum.c:1142:9: note: in expansion of macro ‘MULADDC_INIT’
         MULADDC_INIT
         ^

(This was on Ubuntu 16.04.) Does it work on your machine? Is there something I could be doing wrong?

simonbutcher · 2018-07-02T14:11:00Z

HI @mpg!

I have discovered that PR #1778 doesn't work for -O0, only -O1 upwards to -O3. Can you confirm that's your problem? #1778 will need rework, but it would be nice to confirm that you have the same problem.

cc: @Patater

mpg · 2018-07-03T06:23:18Z

Hi @sbutcher-arm! I can confirm that by adding -O1 to the make invocations for -m32 and -mx32 in all.sh, the tests now pass.

By the way, independently of the current issue with #1778 I think we should always use non-zero -O level because otherwise we're not taking full advantage of -Wall -Wextra -Werror. That is of course, unless the purpose of the test is specifically to make sure that a certain configuration builds with -O0.

simonbutcher · 2018-07-03T08:52:14Z

In my view, Mbed TLS should reasonably build with all settings of -O from 0 to 2 or whatever. I don't think it should fail inexplicably on -O0. The error message isn't self-explanatory at the moment, and it's not clear that that is the fault.

However, the fix in #1778 just gets working again the code that's been in there for years, that worked with the right compiler, so the fix is just meant to get it working again. It's a correction to the design, not a new design, which would be needed. Therefore I think we have two options:

we document in the header that the assembly doesn't work with -O0, and we change all.sh to always use -O2, and we also create a bug to say that i386 doesn't work with -O0.
or, @Patater's team rewrite the assembly to use fewer registers. (And a rewrite isn't going to be done by you or me).

This is @Patater's bug, so what do you think @Patater?

Patater · 2018-07-03T09:10:10Z

Let's avoid using assembly at -O0 for now. Users would want to use assembly for an optimized use case. However, this may limit the testing we do at -O0 as assembly isn't used there. Later, to regain that test coverage, we can consider rewriting to use fewer registers. I've raised #1831 to track the enhancement with minor priority.

simonbutcher · 2018-07-03T09:16:44Z

@mpg suggested over in #1778, that we can wrap the assembly with:

#if defined(__GNUC__) && defined(__OPTIMIZE__)
...inline assembly...
#endif

And if __OPTIMIZE__ isn't defined, meaning -O0 was passed or -O omitted, we can fallback to the C code which will compile everytime.

I think this is the best solution.

simonbutcher · 2018-07-11T07:20:08Z

PR #1831 has now been resubmitted with the fix @mpg suggested above.

@Patater / @mpg - Could you please re-review/re-test this PR?

Patater

Code LGTM

I didn't test it manually.

mpg

I think the test needs improvement. As it stands, it would no longer catch issues with the i386 assembly.

mpg · 2018-07-12T08:32:31Z

tests/scripts/all.sh

-    make CC=gcc CFLAGS='-Werror -Wall -Wextra -m32'
+    cp "$CONFIG_H" "$CONFIG_BAK"
+    scripts/config.pl full
+    make CC=gcc CFLAGS='-Werror -Wall -Wextra -m32 -fsanitize=address'


This builds with -O0, meaning we might not get full warnings, but more importantly we're not testing the assembly code. I think we want to have two builds and tests: one with -O0 and the other with -O1 (with comments explaining why we need the two).

Have added an additional test as suggested.

Review is outdated by rework.

simonbutcher · 2018-07-20T21:08:33Z

@RonEld - Can you please review this again?

RonEld

LGTM

The i386 test builds were only building the default configuration and had no address sanitisation. This commit expands the test configuration to the full configuration in all.sh and builds with ASan for when the test suites are executed.

Added an additional i386 test to all.sh, to allow one test with -O0 which compiles out inline assembly, and one to test with -01 which includes the inline assembly.

The i386 MPI inline assembly code was being incorrectly included when all compiler optimisation was disabled.

simonbutcher · 2018-07-24T07:46:08Z

retest

simonbutcher added enhancement mbed TLS team needs-review Every commit must be reviewed by at least two team members, labels Jun 28, 2018

simonbutcher requested review from mpg and RonEld June 28, 2018 06:49

simonbutcher added the needs-backports Backports are missing or are pending review and approval. label Jun 28, 2018

RonEld reviewed Jun 28, 2018

View reviewed changes

mpg reviewed Jul 2, 2018

View reviewed changes

Patater mentioned this pull request Jul 3, 2018

Rewrite the i386 MPI multiply helper assembly code to use fewer registers #1831

Open

simonbutcher mentioned this pull request Jul 3, 2018

Fix the i386 MPI multiply helper assembly code #1778

Merged

4 tasks

Patater previously approved these changes Jul 12, 2018

View reviewed changes

mpg previously requested changes Jul 12, 2018

View reviewed changes

simonbutcher dismissed Patater’s stale review via 170aa83 July 20, 2018 20:29

Patater previously approved these changes Jul 20, 2018

View reviewed changes

This was referenced Jul 20, 2018

Backport 2.7: Expand i386 all.sh tests to include full configuration ASan builds #1872

Merged

Backport 2.1: Expand i386 all.sh tests to include full configuration ASan builds #1873

Merged

simonbutcher added needs-work and removed needs-backports Backports are missing or are pending review and approval. needs-review Every commit must be reviewed by at least two team members, labels Jul 20, 2018

RonEld previously approved these changes Jul 22, 2018

View reviewed changes

simonbutcher added 3 commits July 22, 2018 18:01

Add additional i386 tests to all.sh

8e6a22a

Added an additional i386 test to all.sh, to allow one test with -O0 which compiles out inline assembly, and one to test with -01 which includes the inline assembly.

Correct logic to exclude i386 inline assenbly when -O0

e459f07

The i386 MPI inline assembly code was being incorrectly included when all compiler optimisation was disabled.

simonbutcher dismissed stale reviews from RonEld and Patater via e459f07 July 23, 2018 15:45

simonbutcher force-pushed the i386-all.sh-fix branch from 170aa83 to e459f07 Compare July 23, 2018 15:45

Patater approved these changes Jul 23, 2018

View reviewed changes

dgreen-arm approved these changes Jul 23, 2018

View reviewed changes

simonbutcher added approved Design and code approved - may be waiting for CI or backports approved for design and removed needs-work labels Jul 23, 2018

simonbutcher merged commit e459f07 into Mbed-TLS:development Jul 24, 2018

hanno-becker mentioned this pull request Jan 3, 2019

ASanDbg i386 build fails due to lack of registers available for bignum assembly #2328

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expand i386 all.sh tests to include full configuration ASan builds #1810

Expand i386 all.sh tests to include full configuration ASan builds #1810

simonbutcher commented Jun 28, 2018 •

edited

Loading

RonEld Jun 28, 2018

simonbutcher Jun 28, 2018

RonEld left a comment

RonEld commented Jun 28, 2018

mpg left a comment

simonbutcher commented Jul 2, 2018

mpg commented Jul 3, 2018

simonbutcher commented Jul 3, 2018

Patater commented Jul 3, 2018

simonbutcher commented Jul 3, 2018

simonbutcher commented Jul 11, 2018

Patater left a comment

mpg left a comment

mpg Jul 12, 2018

simonbutcher Jul 20, 2018

simonbutcher commented Jul 20, 2018

RonEld left a comment

simonbutcher commented Jul 24, 2018

Expand i386 all.sh tests to include full configuration ASan builds #1810

Expand i386 all.sh tests to include full configuration ASan builds #1810

Conversation

simonbutcher commented Jun 28, 2018 • edited Loading

Description

Status

Requires Backporting

Todos

RonEld Jun 28, 2018

Choose a reason for hiding this comment

simonbutcher Jun 28, 2018

Choose a reason for hiding this comment

RonEld left a comment

Choose a reason for hiding this comment

RonEld commented Jun 28, 2018

mpg left a comment

Choose a reason for hiding this comment

simonbutcher commented Jul 2, 2018

mpg commented Jul 3, 2018

simonbutcher commented Jul 3, 2018

Patater commented Jul 3, 2018

simonbutcher commented Jul 3, 2018

simonbutcher commented Jul 11, 2018

Patater left a comment

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

mpg Jul 12, 2018

Choose a reason for hiding this comment

simonbutcher Jul 20, 2018

Choose a reason for hiding this comment

simonbutcher commented Jul 20, 2018

RonEld left a comment

Choose a reason for hiding this comment

simonbutcher commented Jul 24, 2018

simonbutcher commented Jun 28, 2018 •

edited

Loading