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

["Christian Biesinger via gdb-patches" <gdb-patches@sourceware.org>]

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