Re: [PATCH v2] Don't define _FORTIFY_SOURCE on mingw

[Joel Brobecker <brobecker@adacore.com>]

Hi everyone,

On Wed, Dec 18, 2019 at 01:07:05PM -0600, Christian Biesinger via gdb-patches wrote:
> Recent mingw versions require -lssp when using _FORTIFY_SOURCE, which
> gdb does (in common-defs.h)
> https://github.com/msys2/MINGW-packages/issues/5868#issuecomment-544107564
> 
> To avoid all the complications with checking for -lssp and making sure it's
> linked statically, just don't define it.
> 
> gdb/ChangeLog:
> 
> 2019-12-18  Christian Biesinger  <cbiesinger@google.com>
> 
> 	* gdbsupport/common-defs.h: Don't define _FORTIFY_SOURCE on mingw.

Thanks for the patch!

I hestitated a lot on this patch. On the one hand, it wasn't clear
to me that libssp was any different from the other library dependencies
that one can build against... Until it was mentioned that static linking
requires libssp_nonshared! Gaaah... Also, while some people consider it
a necessity to link statically, others would can be in a situation where
it is definitely better for them to link dynamically.

At the end of the day, I think this might be the most sensible approach
after all. I imagine that someone wanting to build with FORTIFY can do
so by adding -D_FORTIFY_SOURCE=2 to the compilation flags, and then
the necessary linking options to add the libraries. For static linking,
one can pass the full path to the archive instead of using -lxxx.
In other words, this patch isn't closing the door for activating
_FORTIFY_SOURCE.

I was also thinking of adding a NEWS entry. However, IIUC, this option
had no effect with previous versions of MinGW? Thus, in the end,
this patch makes the situation stay the same regardless of MinGW
version, meaning no NEWS updated needed.

If my understanding is correct, then I am OK with this patch, and
you can push it to both master and gdb-9-branch. Even if people
object to the approach, this patch doesn't make it any more difficult
to enhance the build to have fortify on by default.

One small comment below...

> 
> Change-Id: Ide6870ab57198219a2ef78bc675768a789ca2b1d
> ---
>  gdb/gdbsupport/common-defs.h | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/gdbsupport/common-defs.h b/gdb/gdbsupport/common-defs.h
> index 203bd8972d..53ce3c96ea 100644
> --- a/gdb/gdbsupport/common-defs.h
> +++ b/gdb/gdbsupport/common-defs.h
> @@ -66,9 +66,13 @@
>     plus this seems like a reasonable safety measure.  The check for
>     optimization is required because _FORTIFY_SOURCE only works when
>     optimization is enabled.  If _FORTIFY_SOURCE is already defined,
> -   then we don't do anything.  */
> +   then we don't do anything.  Also, on mingw, fortify requires

mingw -> MinGW ;-)

> +   linking to -lssp, and to avoid the hassle of checking for
> +   that and linking to it statically, we just don't define
> +   _FORTIFY_SOURCE there.  */
>  
> -#if !defined _FORTIFY_SOURCE && defined __OPTIMIZE__ && __OPTIMIZE__ > 0
> +#if (!defined _FORTIFY_SOURCE && defined __OPTIMIZE__ && __OPTIMIZE__ > 0 \
> +     && !defined(__MINGW32__))
>  #define _FORTIFY_SOURCE 2
>  #endif
>  
> -- 
> 2.24.1.735.g03f4e72817-goog

-- 
Joel