Bug 96321 - make check fails with hardened gcc
Summary: make check fails with hardened gcc
Status: RESOLVED WORKSFORME
Alias: None
Product: valgrind
Classification: Developer tools
Component: general (other bugs)
Version First Reported In: 2.2.0
Platform: Gentoo Packages Linux
: NOR minor
Target Milestone: ---
Assignee: Julian Seward
URL:
Keywords: investigated, triaged
Depends on:
Blocks:
 
Reported: 2005-01-04 22:49 UTC by Maurice van der Pot
Modified: 2018-11-12 16:01 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed/Implemented In:
Sentry Crash Report:


Attachments
Patch to protect ebx while calling clone (1.09 KB, patch)
2005-01-24 19:23 UTC, Tom Hughes
Details
Patch to protect ebx while calling exit (1.09 KB, patch)
2005-01-24 19:25 UTC, Tom Hughes
Details
Patch to protect ebx while calling exit (1.14 KB, patch)
2005-01-25 01:11 UTC, Tom Hughes
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Maurice van der Pot 2005-01-04 22:49:36 UTC
When using gcc hardened, valgrind fails make check with the following message:

if i686-pc-linux-gnu-gcc -DHAVE_CONFIG_H -I. -I. -I..     -Winline -Wall
-Wshadow -g  -MT cputest.o -MD -MP -MF ".deps/cputest.Tpo" -c -o cputest.o
cputest.c; \
then mv -f ".deps/cputest.Tpo" ".deps/cputest.Po"; else rm -f
".deps/cputest.Tpo"; exit 1; fi
cputest.c: In function `cpuid':
cputest.c:9: error: can't find a register in class `BREG' while reloading `asm'
make[4]: *** [cputest.o] Error 1
make[4]: Leaving directory
`/var/tmp/portage/valgrind-2.2.0-r1/work/valgrind-2.2.0/tests'
make[3]: *** [check-am] Error 2

I checked bugzilla and temporarily applied the patch from comment #3 on bug
#79696, but afterwards I got these errors:

make[4]: Entering directory
`/var/tmp/portage/valgrind-2.2.0-r1/work/valgrind-2.2.0/none/tests'
...
if i686-pc-linux-gnu-gcc -DHAVE_CONFIG_H -I. -I. -I../..     -Winline -Wall
-Wshadow -g -I../../include  -MT insn_basic.o -MD -MP -MF ".deps/insn_basic.Tpo"
-c -o insn_basic.o insn_basic.c; \
then mv -f ".deps/insn_basic.Tpo" ".deps/insn_basic.Po"; else rm -f
".deps/insn_basic.Tpo"; exit 1; fi
insn_basic.c: In function `adcb_3':
insn_basic.c:1168: error: PIC register `bl' clobbered in `asm'
insn_basic.c: In function `adcb_4':
insn_basic.c:1209: error: PIC register `bl' clobbered in `asm'
insn_basic.c: In function `adcb_7':
insn_basic.c:1329: error: PIC register `bl' clobbered in `asm'
insn_basic.c: In function `adcb_8':
insn_basic.c:1372: error: PIC register `bl' clobbered in `asm'
insn_basic.c: In function `adcb_11':
insn_basic.c:1497: error: PIC register `bl' clobbered in `asm'
insn_basic.c: In function `adcb_12':
insn_basic.c:1539: error: PIC register `bl' clobbered in `asm'
insn_basic.c: In function `adcw_5':
insn_basic.c:1744: error: PIC register `bx' clobbered in `asm'
insn_basic.c: In function `adcw_6':
insn_basic.c:1785: error: PIC register `bx' clobbered in `asm'
insn_basic.c: In function `adcw_9':
insn_basic.c:1905: error: PIC register `bx' clobbered in `asm'
insn_basic.c: In function `adcw_10':
insn_basic.c:1948: error: PIC register `bx' clobbered in `asm'
insn_basic.c: In function `adcw_13':
insn_basic.c:2073: error: PIC register `bx' clobbered in `asm'
insn_basic.c: In function `adcw_14':
insn_basic.c:2115: error: PIC register `bx' clobbered in `asm'
insn_basic.c: In function `adcl_5':
insn_basic.c:2320: error: PIC register `ebx' clobbered in `asm'
insn_basic.c: In function `adcl_6':
...

The makefiles don't seem to respect -nopie/-fno-pic
Comment 1 Jeremy Fitzhardinge 2005-01-20 20:09:38 UTC
I added a --without-pie option to configure.  Is that enough?

The tests should probably never be built with -fpie anyway.
Comment 2 Maurice van der Pot 2005-01-21 21:39:42 UTC
It does not solve the problem. To turn off default PIE building with hardened
gcc, -nopie has to be added to CFLAGS.

Even though I have CFLAGS=-nopie in the environment, it is not used on the 
compiler command line. The only way it currently works is like this:
  make CFLAGS=-nopie

Unlike CFLAGS, CXXFLAGS specified in the environment are taken over properly.
Comment 3 Tom Hughes 2005-01-23 17:54:34 UTC
I could add -nopie to the CFLAGS for cputest, but it would break building on older systems where gcc doesn't have PIE support and therefore doesn't understand the flag.

The best thing to do would be to replace the inline assembler with out of line assember as I did in the valgrind core - calling cpuid from inline assembler is quite hard because it uses so many registers that gcc often winds up failing due to not being able to release enough registers for the assembler.
Comment 4 Tom Hughes 2005-01-23 18:10:07 UTC
CVS commit by thughes: 

Link in libarch.a and use VG_(cpuid) for x86 CPU feature tests rather
than trying to use our own inline version which has a habit of failing
to compile on some versions of gcc due to register starvation.

BUG: 96321


  M +2 -1      Makefile.am   1.38
  M +9 -8      cputest.c   1.7


--- valgrind/tests/Makefile.am  #1.37:1.38
@@ -23,5 +23,6 @@
 cputest_SOURCES         = cputest.c
 cputest_CFLAGS          = $(AM_CFLAGS) -D__$(VG_ARCH)__
+cputest_DEPENDENCIES    = ../coregrind/${VG_ARCH}/libarch.a
+cputest_LDADD           = ../coregrind/${VG_ARCH}/libarch.a
 toobig_allocs_SOURCES   = toobig-allocs.c
 true_SOURCES            = true.c
-

--- valgrind/tests/cputest.c  #1.6:1.7
@@ -23,13 +23,14 @@ char* all_archs[] = {
 
 #ifdef __x86__
-static __inline__ void cpuid(unsigned int n,
+extern void vgPlain_cpuid(unsigned int n,
+                          unsigned int *a, unsigned int *b,
+                          unsigned int *c, unsigned int *d);
+
+static inline void cpuid(unsigned int n,
                              unsigned int *a, unsigned int *b,
                              unsigned int *c, unsigned int *d)
 {
-   __asm__ __volatile__ (
-      "cpuid"
-      : "=a" (*a), "=b" (*b), "=c" (*c), "=d" (*d)      /* output */
-      : "0" (n)         /* input */
-   );
+   vgPlain_cpuid(n, a, b, c, d);
+   return;
 }
 


Comment 5 Maurice van der Pot 2005-01-23 20:56:24 UTC
I'm sorry, but this bug has not been fixed yet.

I think the problem in this case is that BREG is reserved. I don't know much
about hardened gcc myself, but every time I get a message, it's about BREG.

For instance, latest CVS fails to build with this error:

syscalls.c: In function `start_thread':
syscalls.c:245: error: can't find a register in class `BREG' while reloading `asm'
make[2]: *** [syscalls.o] Error 1

I do not mind having to put -nopie in CFLAGS, I can do that, but right now it
has no effect on the actual flags being used to compile. If that could be fixed,
it would be an acceptable solution for me. 

If at some time it will become possible to build everything with -pie, I'll 
remove -nopie from CFLAGS. I just need a work-around until that time.
Comment 6 Tom Hughes 2005-01-24 01:10:42 UTC
That'a different bug! Your original complaint was what you couldn't test the software, which implies that you had managed to build it. As far as I know that is resolved and if you now have a problem building the software then that is something completely different and you should open a new bug.
Comment 7 Maurice van der Pot 2005-01-24 18:34:43 UTC
I could of course have entered 3 bugs; all of which would have referred to the 
problem that ebx is used in inline asm and hardened gcc reserves ebx for 
something else; all of which could have been worked around if the makefiles
took CFLAGS in the environment into account.

In my view these three are only symptoms of a common root cause and imo one root
cause means one bug. It's a bug report, not a problem report.
If you want to have them reported separately, that's fine with me. 

In any case, this report has not been handled completely. 
2 of the 3 problems prevented me from testing valgrind and I mentioned them
both in my original report. Only the first one (cpuid) has been solved.
Comment 8 Tom Hughes 2005-01-24 19:16:10 UTC
The problem is that they're very different bugs that need different sort of fixes and different people to look at them - the two on the original report are things I can look at for example but the other one is best looked at by Jeremy. It's very hard to manage the bugs if they lots of bits and need to be passed from person to person before they can be considered resolved.

I hadn't even noticed the second problem in your original report for example because I got far enough through reading it to see that cputest was failing and didn't read what I thought was just more follow on errors from the compiler.

Fixing the insn tests may just be a matter of reoving the ebx registers from the list of those the test generator will use, but that may mean we run out of registers so I will need to play with that.

I have no idea what to do about the start thread problem - we can't control the fact that the clone system call returns a result in ebx and we need that result. I guess we will have to save ebx before the call and then move the result to another register and restore ebx afterwards.

Frankly it all sounds as if your compiler is pretty horribly broken. I've never encounterd a gcc that completely refused to let inline assembler use the b registers before. What is a hardened gcc when it's at home anyway?
Comment 9 Tom Hughes 2005-01-24 19:23:16 UTC
Created attachment 9265 [details]
Patch to protect ebx while calling clone

Does this patch help with the start_thread problem? It attempts to avoid
relying on ebx in the inline assembler used to call clone. I'm still looking at
the instruction test problem.
Comment 10 Tom Hughes 2005-01-24 19:25:24 UTC
Created attachment 9266 [details]
Patch to protect ebx while calling exit

Fixed patch to pop instead of pushing at the end... Also, it is the exit call
it is protecting not the clone.
Comment 11 Jeremy Fitzhardinge 2005-01-24 19:27:12 UTC
Hm, you're restoring the value of %ebx after exit() returns...
Comment 12 Tom Hughes 2005-01-25 01:11:26 UTC
Created attachment 9276 [details]
Patch to protect ebx while calling exit

Indeed I am - if exit does somehow fail and we reach that panic then we need to
protect ebx surely? What was wrong was that I was treating ebx as a return
value instead of an input. This version of the patch fixes that.
Comment 13 Jeremy Fitzhardinge 2005-01-25 03:38:20 UTC
As the comment implies, if that exit fails, we're unbelievably screwed because there may not be a stack (or more to the point, it may have been recycled by another thread).  It would be better to just put "ud2" after the int $0x80.

I'm also not too convinced we should spend too much effort on this hardened-gcc thing.  It doesn't seem terribly useful for Valgrind; certainly not enough to uglify things for its sake.
Comment 14 Maurice van der Pot 2005-01-25 18:13:00 UTC
I'll just say that for me it is enough if I can pass CFLAGS through the 
environment. That way you guys don't have to change any code and I can just
use -nopie for building valgrind.
Comment 15 Maurice van der Pot 2005-02-01 20:55:41 UTC
configure.in contains the following few lines:

----------------------------------8<----------------------------------

# Checks for programs.
CFLAGS=""

----------------------------------8<----------------------------------

If I comment out the line clearing CFLAGS, the build uses the flags I set 
in the environment. This enables me to make/make check without problems.

I don't know why CFLAGS is cleared. It was added in version 1.2 of configure.in.
Comment 16 Maurice van der Pot 2005-04-11 00:01:19 UTC
In response to Jeremy in comment #1 and Tom in #3:
You could write a test in configure for the existence of the -fno-pie flag and
then add it conditionally to CFLAGS in the makefiles of the tests.

In response to #12 and #13:
I'm not sure which solution would be better, but I used the provided patch for
now and valgrind itself builds without having to disable pie.

This would take care of the build failures when using a compiler that defaults 
to -fpie.
Comment 17 Nicholas Nethercote 2009-07-02 06:16:30 UTC
I'm closing crashing and similar bugs that are more than two years old.  If
you still see this problem with Valgrind 3.4.1 please reopen the bug report.
Thanks.
Comment 18 Andrew Crouthamel 2018-09-19 04:35:05 UTC
Dear Bug Submitter,

This bug has been in NEEDSINFO status with no change for at least 15 days. Please provide the requested information as soon as possible and set the bug status as REPORTED. Due to regular bug tracker maintenance, if the bug is still in NEEDSINFO status with no change in 30 days the bug will be closed as RESOLVED > WORKSFORME due to lack of needed information.

For more information about our bug triaging procedures please read the wiki located here: https://community.kde.org/Guidelines_and_HOWTOs/Bug_triaging

If you have already provided the requested information, please mark the bug as REPORTED so that the KDE team knows that the bug is ready to be confirmed.

Thank you for helping us make KDE software even better for everyone!
Comment 19 Bug Janitor Service 2018-11-12 16:01:01 UTC
This bug has been in NEEDSINFO status with no change for at least
30 days. The bug is now closed as RESOLVED > WORKSFORME
due to lack of needed information.

For more information about our bug triaging procedures please read the
wiki located here:
https://community.kde.org/Guidelines_and_HOWTOs/Bug_triaging

Thank you for helping us make KDE software even better for everyone!