[v2] Don't define _FORTIFY_SOURCE on mingw

  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.

Change-Id: Ide6870ab57198219a2ef78bc675768a789ca2b1d
---
 gdb/gdbsupport/common-defs.h | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
  

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
  

Hi Joel,

On Thu, Jan 2, 2020 at 5:31 AM Joel Brobecker <brobecker@adacore.com> wrote:
>
> 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.

Sorry, to clarify the issue there -- libssp_nonshared is not needed,
what is needed are the right linker flags:
 -Wl,-Bstatic -lssp -Wl,-Bdynamic
(since, per Eli, we want to link this statically)

However, that means I can't just use AC_SEARCH_LIBS. So I'd have to
either use AC_LINK_IFELSE, which is more complicated, or hardcode
those flags if target matches mingw (doesn't sound too great).

> 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.

Yes, that is true.

> 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.

My understanding is that this did use to work (maybe it automatically
linked against -lssp?). But I'm not sure a NEWS entry is needed? This
does not seem user-visible.

> 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.

Let me know your thoughts on the above, especially the NEWS question.

> 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 ;-)

Thanks, will fix locally.

Christian
  

> From: "Christian Biesinger via gdb-patches" <gdb-patches@sourceware.org>
> Date: Tue, 7 Jan 2020 17:42:55 -0600
> Cc: Christian Biesinger via gdb-patches <gdb-patches@sourceware.org>
> 
> > 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.
> 
> My understanding is that this did use to work (maybe it automatically
> linked against -lssp?). But I'm not sure a NEWS entry is needed? This
> does not seem user-visible.

I think it used to work because the MinGW GCC wasn't reacting to
_FORTIFY_SOURCE, so it was largely a no-op for MinGW.  This changed in
a recent version of MinGW GCC.

I'm also not sure this should be in NEWS.  It depends on how MinGW
users who build GDB are sensitive to _FORTIFY_SOURCE.
  

>>>>> "Christian" == Christian Biesinger via gdb-patches <gdb-patches@sourceware.org> writes:

>> 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.

Christian> Let me know your thoughts on the above, especially the NEWS question.

I think it is fine as-is, without a NEWS entry.
Please check it in.

thanks,
Tom
  

On Thu, Jan 9, 2020 at 4:23 PM Tom Tromey <tom@tromey.com> wrote:
>
> >>>>> "Christian" == Christian Biesinger via gdb-patches <gdb-patches@sourceware.org> writes:
>
> >> 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.
>
> Christian> Let me know your thoughts on the above, especially the NEWS question.
>
> I think it is fine as-is, without a NEWS entry.
> Please check it in.

Thanks, pushed to master. OK for the gdb 9 branch too?

To ssh://sourceware.org/git/binutils-gdb.git
   3061113bf3..5f23a08201  HEAD -> master

Christian
  

Christian> Thanks, pushed to master. OK for the gdb 9 branch too?

Yes, please.

thanks,
Tom
  

On Thu, Jan 9, 2020 at 6:28 PM Tom Tromey <tom@tromey.com> wrote:
>
> Christian> Thanks, pushed to master. OK for the gdb 9 branch too?
>
> Yes, please.

Thanks, done.
To ssh://sourceware.org/git/binutils-gdb.git
   ad5e26527f..975292a976  HEAD -> gdb-9-branch

Christian