Patches to compile with CPPFLAGS=-D_FORTIFY_SOURCE=2

Strictly for discussing ZSNES development and for submitting code. You can also join us on IRC at irc.freenode.net in #zsnes.
Please, no requests here.

Moderator: ZSNES Mods

Post Reply
fabian
Rookie
Posts: 14
Joined: Wed Jul 04, 2012 2:24 pm

Patches to compile with CPPFLAGS=-D_FORTIFY_SOURCE=2

Post by fabian »

Dear ZSNES devs,

I'd like to get your (dis)approval for a patch set that I am about to apply to the zsnes package in Debian.

The underlying problem is that Debian encourages to build package with "hardening build flags", i.e. compiler flags that add security-related features like stack protection. These flags also add "-D_FORTIFY_SOURCE=2" to CPPFLAGS, which cause zsnes to fail to build. An analysis of the problem is given by ubuntu here:
https://bugs.launchpad.net/ubuntu/intrepid/+source/zsnes/+bug/250425/comments/45

However, Fedora, nee RPM Fusion, provides a patch for it:
http://cvs.rpmfusion.org/viewvc/rpms/zsnes/devel/zsnes-1.51-FORTIFY_SOURCE.patch?revision=1.1&root=free&view=markup

With this patch, zsnes compiles fine with GCC 4.6.x and "-D_FORTIFY_SOURCE=2" enabled in CPPFLAGS, although it spits a lot of warnings. However, when the compiler is updated to GCC 4.7.x, compilation breaks again for code that is produced by parsegen, so an additional patch is needed to omit the direct creation of object code from parsegen but create intermediate .c-files instead:
http://cvs.rpmfusion.org/viewvc/rpms/zsnes/devel/zsnes-1.51-psr.patch?revision=1.1&root=free&view=markup

As you know the affected code best, could you please do me a favour and share your opinion about these two patches?

Best regards,
Fabian
Nach
ZSNES Developer
ZSNES Developer
Posts: 3903
Joined: Tue Jul 27, 2004 10:54 pm
Location: Solar powered park bench
Contact:

Re: Patches to compile with CPPFLAGS=-D_FORTIFY_SOURCE=2

Post by Nach »

The patches proposed by Fedora for memcpy() indeed fix the "problem" however they misrepresent the variables with their changes. We're aware that GCC is unable to see all of ZSNES assembly code, so it outputs warnings for situations that in "normal" code would be a problem.

The correct solution would be to port of all of ZSNES's assembly code there to C, but that's a huge job, which is only partially done.

If you want to use the patches to initc.c and zstate.c to quiet some warnings, I *think* they're okay, but we wouldn't include them in ZSNES itself. I say "think", because I haven't checked to see if they're actually used anywhere other than in memcpy() routines.

Regarding the patch to parsegen, we know that such a change will cause certain versions of GCC to produce invalid code with certain system CFLAGS. You'll note that not only does parsegen compile internally, but after the CFLAGS it adds to the end -O1. We found certain versions of GCC were producing incorrect code at -O2 and -O3 for the C files parsegen produces.
May 9 2007 - NSRT 3.4, now with lots of hashing and even more accurate information! Go download it.
_____________
Insane Coding
fabian
Rookie
Posts: 14
Joined: Wed Jul 04, 2012 2:24 pm

Re: Patches to compile with CPPFLAGS=-D_FORTIFY_SOURCE=2

Post by fabian »

Nach wrote:Regarding the patch to parsegen, we know that such a change will cause certain versions of GCC to produce invalid code with certain system CFLAGS. You'll note that not only does parsegen compile internally, but after the CFLAGS it adds to the end -O1. We found certain versions of GCC were producing incorrect code at -O2 and -O3 for the C files parsegen produces.


I have just made a different experience and am not sure if it is in contradiction to your statement above.

Currently, with -O1 added to the system CFLAGS, parsegen fails to compile internally. If I remove the additional -O1 and compile only with the system CFLAGS (which have got -O3 added by the ZSNES Makefile, BTW), the code compiles cleanly. It does, however, also succeed to compile internally if I leave the additional -O1 in place and simply add "-fipa-cp".
Do you consider this safe?

- Fabian
Nach
ZSNES Developer
ZSNES Developer
Posts: 3903
Joined: Tue Jul 27, 2004 10:54 pm
Location: Solar powered park bench
Contact:

Re: Patches to compile with CPPFLAGS=-D_FORTIFY_SOURCE=2

Post by Nach »

fabian wrote:Currently, with -O1 added to the system CFLAGS, parsegen fails to compile internally.

I find that extremely odd. Can you paste for me the line it's trying to use to compile with?

fabian wrote:If I remove the additional -O1 and compile only with the system CFLAGS (which have got -O3 added by the ZSNES Makefile, BTW), the code compiles cleanly.

Which however does not mean that the compilation is correct.

fabian wrote:It does, however, also succeed to compile internally if I leave the additional -O1 in place and simply add "-fipa-cp".
Do you consider this safe?

I'm not famialiar with that GCC option. I've been searching for some documentation for a couple of minutes now, but to no avail.
May 9 2007 - NSRT 3.4, now with lots of hashing and even more accurate information! Go download it.
_____________
Insane Coding
fabian
Rookie
Posts: 14
Joined: Wed Jul 04, 2012 2:24 pm

Re: Patches to compile with CPPFLAGS=-D_FORTIFY_SOURCE=2

Post by fabian »

Nach wrote:I find that extremely odd. Can you paste for me the line it's trying to use to compile with?


./parsegen -D__UNIXSDL__ -D__OPENGL__ -gcc gcc -compile -flags "-g -O2 -fstack-protector --param=ssp-buffer-size=4 -Wformat -Werror=format-security -D_FORTIFY_SOURCE=2 -pipe -I. -I/usr/local/include -I/usr/include -D__UNIXSDL__ -I/usr/include/SDL -D_GNU_SOURCE=1 -D_REENTRANT -D__LIBAO__ -D__OPENGL__ -march=i486 -O3 -fomit-frame-pointer -fprefetch-loop-arrays -fforce-addr -D__RELEASE__ -O1" -cheader cfg.h -fname cfg cfg.o cfg.psr
parsegen: gcc -g -O2 -fstack-protector --param=ssp-buffer-size=4 -Wformat -Werror=format-security -D_FORTIFY_SOURCE=2 -pipe -I. -I/usr/local/include -I/usr/include -D__UNIXSDL__ -I/usr/include/SDL -D_GNU_SOURCE=1 -D_REENTRANT -D__LIBAO__ -D__OPENGL__ -march=i486 -O3 -fomit-frame-pointer -fprefetch-loop-arrays -fforce-addr -D__RELEASE__ -O1 -o cfg.o -c cfg.c
cfg.c:1:0: warning: -fprefetch-loop-arrays not supported for this target (try -march switches) [enabled by default]
In file included from /usr/include/stdio.h:930:0,
from cfg.c:5:
cfg.c: In function ‘write_cfg_vars’:
/usr/include/i386-linux-gnu/bits/stdio2.h:96:1: error: inlining failed in call to always_inline ‘fprintf’: indirect function call with a yet undetermined callee
cfg.c:552:7: error: called from here
In file included from /usr/include/stdio.h:930:0,
from cfg.c:5:
/usr/include/i386-linux-gnu/bits/stdio2.h:96:1: error: inlining failed in call to always_inline ‘fprintf’: indirect function call with a yet undetermined callee
[...]

I'm not famialiar with that GCC option. I've been searching for some documentation for a couple of minutes now, but to no avail.


-fipa-cp
Perform interprocedural constant propagation. This optimization analyzes the program to determine when values passed to functions are constants and then optimizes accordingly. This optimization can substantially increase performance if the application has constants passed to functions. This flag is enabled by default at -O2, -Os and -O3.
http://gcc.gnu.org/onlinedocs/gcc-4.7.1/gcc/Optimize-Options.html#Optimize-Options
Nach
ZSNES Developer
ZSNES Developer
Posts: 3903
Joined: Tue Jul 27, 2004 10:54 pm
Location: Solar powered park bench
Contact:

Re: Patches to compile with CPPFLAGS=-D_FORTIFY_SOURCE=2

Post by Nach »

In general, your flags look odd. -g? -O2?

In any case, I played with your line, the exact issue seems to occur with -O1 combined with -D_FORTIFY_SOURCE=2. Smells like yet another GCC bug. Although probably can put some blame on EGLIBC as well, as it changes how stdio works with fortify source, and is defining various functions as requiring inline.

I looked up -fipa-cp, and it seems it was introduced in GCC 4.3, which is around the same time we first started noticing issues with parsegen generated files being compiled at -O2 and -O3. Since -O2 and higher turns it on, for all I know, it's the cause, I haven't tested it.

I noticed that using -D_FORTIFY_SOURCE=2 with -O0 compiles. I don't recall though if we ever verified -O0 as working properly. Also based on how different the code is generated with all these options, I wonder if yet another GCC bug isn't hiding here. I think we'll need someone to go over the GCC generated code and ensure no "else if" statements are being deleted like we found in the past. Or alternatively, testing if all changes in the config file propagate properly to ZSNES in action, to ensure that the config file is indeed parsed as intended.
May 9 2007 - NSRT 3.4, now with lots of hashing and even more accurate information! Go download it.
_____________
Insane Coding
fabian
Rookie
Posts: 14
Joined: Wed Jul 04, 2012 2:24 pm

Re: Patches to compile with CPPFLAGS=-D_FORTIFY_SOURCE=2

Post by fabian »

Nach wrote:In general, your flags look odd. -g? -O2?

These belong to the standard build flags in Debian.

Anyway, thanks for your analysis. For the time being, I just removed the trailing -O1 from the parsegen command line. This has the same effect as the Fedora patch, which avoinds compilation inside parsegen and leads to the creation intermediate .c files which are then compiled into object code with the standard flags.
grinvader
ZSNES Shake Shake Prinny
Posts: 5626
Joined: Wed Jul 28, 2004 4:15 pm
Location: PAL50, dood !

Re: Patches to compile with CPPFLAGS=-D_FORTIFY_SOURCE=2

Post by grinvader »

Can't say having -g as systemwide default is a good move, but YMMV I guess.
皆黙って俺について来い!!

Code: Select all

<jmr> bsnes has the most accurate wiki page but it takes forever to load (or something)

Pantheon: Gideon Zhi | CaitSith2 | Nach | kode54
Post Reply