Message ID | 20191218190705.161582-1-cbiesinger@google.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 66798 invoked by alias); 18 Dec 2019 19:07:11 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 66773 invoked by uid 89); 18 Dec 2019 19:07:11 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-21.8 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.1 spammy= X-HELO: mail-qk1-f202.google.com Received: from mail-qk1-f202.google.com (HELO mail-qk1-f202.google.com) (209.85.222.202) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 18 Dec 2019 19:07:10 +0000 Received: by mail-qk1-f202.google.com with SMTP id l7so2000928qke.8 for <gdb-patches@sourceware.org>; Wed, 18 Dec 2019 11:07:09 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:in-reply-to:message-id:mime-version:references:subject:from:to :cc; bh=nNllV8YYtk6yT1EhMvBD1ihCmWh5z4UNfTQ0Z2H0ess=; b=k5X0PC8Fw1g1KnCAD+Dpht9DrepuNzGNXF2TrOwQIdHT3Aeup3ESJq58yqoOaxiDT0 rlqvBi5w6To1HbVReg0ehRbcreZj20WqymKx3Y2ddctjo79xI8JKfpaUL1T9epfsVZTo +vxAEIPu/n2o/baPXez5viLQGV6IVwGw4J69QtniDA3XAhWylr+N3XYwgELR+0ijoM3O efBmWBnMl5qHCcISNamWcWxa9mwe4mfcdhcB9Od4iBPo354QOf/ANUBPRzy3aDWwO+b2 yhDtbSdanUC7vQej5yoyQ9MHWMIkX/rQByvLkRpGPBThdMDk4U/qm7Y/K2rDhpF7xoBD p1bg== Date: Wed, 18 Dec 2019 13:07:05 -0600 In-Reply-To: <CAPTJ0XHS3mhnBzUmN0X1wJZMH69V_aQQPH7Trxf9TnQMfgH0ng@mail.gmail.com> Message-Id: <20191218190705.161582-1-cbiesinger@google.com> Mime-Version: 1.0 References: <CAPTJ0XHS3mhnBzUmN0X1wJZMH69V_aQQPH7Trxf9TnQMfgH0ng@mail.gmail.com> Subject: [PATCH v2] Don't define _FORTIFY_SOURCE on mingw From: "Christian Biesinger via gdb-patches" <gdb-patches@sourceware.org> Reply-To: Christian Biesinger <cbiesinger@google.com> To: gdb-patches@sourceware.org Cc: Christian Biesinger <cbiesinger@google.com> Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes |
Commit Message
Terekhov, Mikhail via Gdb-patches
Dec. 18, 2019, 7:07 p.m. UTC
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(-)
Comments
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
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 + 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