Oh dear. I regret to inform you that commit 'RAII-fy make_cleanup_restore_current_thread & friends' might be unfortunate
Message ID | 79511435-a3a8-48f9-2e16-bca8adb1909d@redhat.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 1269 invoked by alias); 4 May 2017 18:23:08 -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 1216 invoked by uid 89); 4 May 2017 18:23:06 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.6 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_ASCII_DIVIDERS, RCVD_IN_DNSWL_NONE, RCVD_IN_SORBS_SPAM autolearn=ham version=3.3.2 spammy=H*r:192.168.43, folks, regret X-HELO: mail-wm0-f52.google.com Received: from mail-wm0-f52.google.com (HELO mail-wm0-f52.google.com) (74.125.82.52) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 04 May 2017 18:23:03 +0000 Received: by mail-wm0-f52.google.com with SMTP id u65so4514634wmu.1 for <gdb-patches@sourceware.org>; Thu, 04 May 2017 11:23:05 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=ESDbEi+Jf84plkukgbSpDCaFraBoKw1p5kODUXlN3aw=; b=Fp320lGN4pEYvjizIfynuiCTP1gbcbF2TmKdH2fr2TaZjCVAcZUq3/UUdP01wbKcig CbCw7x1/7u3e4FihkKxdYndmFzYcfu0s7h/5/iVEta8uVDbVJJdU/PMgI56lpwAsXdHJ wkO3qSaUVI1xaPz6pxtD//mgDK4WT34htC+bnZjn73jmvXZKHOeTNCzqmDgZujOVenbk iD7bz2y3xlj8KIOFOUWMjRasA+iw57SrP50i87ByiJp/OsMH99zrr3XiHB3jdU5roun6 XxVioTH/NX9UBEyz2lZRNuS3bjZHp3we6LKn80gRzk7/JPlg3Q26yv6GDApMgsOcyyDQ bByg== X-Gm-Message-State: AN3rC/7EzNHhA71Aq8WD16gL5bjFUmRhqaGf/67OgnQp1KAE7OdZLpZT 2V8ACmXbPsUjsrLx1RJgq242 X-Received: by 10.28.91.77 with SMTP id p74mr2785087wmb.54.1493922183894; Thu, 04 May 2017 11:23:03 -0700 (PDT) Received: from [192.168.43.236] ([95.69.93.66]) by smtp.gmail.com with ESMTPSA id t85sm2368373wmt.23.2017.05.04.11.23.01 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 04 May 2017 11:23:02 -0700 (PDT) Subject: Re: Oh dear. I regret to inform you that commit 'RAII-fy make_cleanup_restore_current_thread & friends' might be unfortunate To: Simon Marchi <simon.marchi@polymtl.ca> References: <E1d6JGc-0000zB-OE@kwanyin.sergiodj.net> <34792dd0-088c-a1d1-9125-70c8585c21bd@redhat.com> <6bf88edee0fb17451d44b85bb00fb0d0@polymtl.ca> Cc: gdb-patches@sourceware.org From: Pedro Alves <palves@redhat.com> Message-ID: <79511435-a3a8-48f9-2e16-bca8adb1909d@redhat.com> Date: Thu, 4 May 2017 19:22:58 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <6bf88edee0fb17451d44b85bb00fb0d0@polymtl.ca> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit |
Commit Message
Pedro Alves
May 4, 2017, 6:22 p.m. UTC
On 05/04/2017 07:15 PM, Simon Marchi wrote: > On 2017-05-04 12:42, Pedro Alves wrote: >> On 05/04/2017 05:06 PM, gdb-buildbot@sergiodj.net wrote: >>> g++ -g -O2 -I. -I../../binutils-gdb/gdb >>> -I../../binutils-gdb/gdb/common -I../../binutils-gdb/gdb/config >>> -DLOCALEDIR="\"/usr/local/share/locale\"" -DHAVE_CONFIG_H >>> -I../../binutils-gdb/gdb/../include/opcode >>> -I../../binutils-gdb/gdb/../opcodes/.. >>> -I../../binutils-gdb/gdb/../readline/.. >>> -I../../binutils-gdb/gdb/../zlib -I../bfd >>> -I../../binutils-gdb/gdb/../bfd -I../../binutils-gdb/gdb/../include >>> -I../libdecnumber -I../../binutils-gdb/gdb/../libdecnumber >>> -I../../binutils-gdb/gdb/gnulib/import -Ibuild-gnulib/import >>> -DTUI=1 -pthread -I/usr/include/guile/2.0 -I/usr/include/python2.7 >>> -I/usr/include/python2.7 -Wall -Wpointer-arith -Wno-unused >>> -Wunused-value -Wunused-function -Wno-switch -Wno-char-subscripts >>> -Wempty-body -Wunused-but-set-parameter -Wunused-but-set-variable >>> -Wno-sign-compare -Wno-narrowing -Wformat-nonliteral -Werror -c -o >>> interps.o -MT interps.o -MMD -MP -MF .deps/interps.Tpo >>> ../../binutils-gdb/gdb/interps.c >>> In file included from ../../binutils-gdb/gdb/infrun.c:26:0: >>> ../../binutils-gdb/gdb/inferior.h: In function void >>> handle_vfork_child_exec_or_exit(int): >>> ../../binutils-gdb/gdb/inferior.h:553:39: error: *((void*)(& >>> maybe_restore_inferior)+40).scoped_restore_current_inferior::m_saved_inf >>> may be used uninitialized in this function [-Werror=maybe-uninitialized] >>> { set_current_inferior (m_saved_inf); } >>> ^ >>> ../../binutils-gdb/gdb/infrun.c:940:6: note: *((void*)(& >>> maybe_restore_inferior)+40).scoped_restore_current_inferior::m_saved_inf >>> was declared here >>> maybe_restore_inferior; >>> ^~~~~~~~~~~~~~~~~~~~~~ >>> In file included from ../../binutils-gdb/gdb/inferior.h:46:0, >>> from ../../binutils-gdb/gdb/infrun.c:26: >>> ../../binutils-gdb/gdb/progspace.h:274:47: error: *((void*)(& >>> maybe_restore_inferior)+32).scoped_restore_current_program_space::m_saved_pspace >>> may be used uninitialized in this function [-Werror=maybe-uninitialized] >>> { set_current_program_space (m_saved_pspace); } >>> ^ >>> ../../binutils-gdb/gdb/infrun.c:940:6: note: *((void*)(& >>> maybe_restore_inferior)+32).scoped_restore_current_program_space::m_saved_pspace >>> was declared here >>> maybe_restore_inferior; >>> ^~~~~~~~~~~~~~~~~~~~~~ >> >> Fun, looks like gdb::optional related ... >> >> Thanks, >> Pedro Alves > > What do you need to reproduce this? GCC 7? The above is g++ 6.3.1, if I can trust the buildslave description page. I didn't try that release, but gcc master warns the same way. And, if I hack the code to use std::optional instead of gdb::optional : and compile with -std=gnu++17, I get the exact same warning again... src/gdb/infrun.c In file included from /home/pedro/gdb/mygit/cxx-convertion/src/gdb/infrun.c:26:0: /home/pedro/gdb/mygit/cxx-convertion/src/gdb/inferior.h: In function ‘void handle_vfork_child_exec_or_exit(int)’: /home/pedro/gdb/mygit/cxx-convertion/src/gdb/inferior.h:553:26: error: ‘*((void*)(& maybe_restore_inferior)+40).scoped_restore_current_inferior::m_saved_inf’ may be used uninitialized in this function [-Werror=maybe-uninitialized] { set_current_inferior (m_saved_inf); } ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~ /home/pedro/gdb/mygit/cxx-convertion/src/gdb/infrun.c:942:6: note: ‘*((void*)(& maybe_restore_inferior)+40).scoped_restore_current_inferior::m_saved_inf’ was declared here maybe_restore_inferior; ^~~~~~~~~~~~~~~~~~~~~~ In file included from /home/pedro/gdb/mygit/cxx-convertion/src/gdb/inferior.h:46:0, from /home/pedro/gdb/mygit/cxx-convertion/src/gdb/infrun.c:26: /home/pedro/gdb/mygit/cxx-convertion/src/gdb/progspace.h:274:31: error: ‘*((void*)(& maybe_restore_inferior)+32).scoped_restore_current_program_space::m_saved_pspace’ may be used uninitialized in this function [-Werror=maybe-uninitialized] { set_current_program_space (m_saved_pspace); } ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~ /home/pedro/gdb/mygit/cxx-convertion/src/gdb/infrun.c:942:6: note: ‘*((void*)(& maybe_restore_inferior)+32).scoped_restore_current_program_space::m_saved_pspace’ was declared here maybe_restore_inferior; ^~~~~~~~~~~~~~~~~~~~~~ cc1plus: all warnings being treated as errors AFAICS so far, this is a false positive. Not sure what to do. I wouldn't want to force-memset the optional's storage to work around it, which would be a pessimization to quiet a warning. From above, we see that that wouldn't work when we later start using std::optional. There's a bug open about this (for boost::optional, but most probably the exact same): https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78044 Comment #2 makes me think that we should really disable the warning, or at least make it "-Wno-error=maybe-uninitialized". I've seen other similar comments around the interwebs when looking for this warning + optional. Maybe I could check if the gcc folks plan on doing something to std::optional to work around this. Thanks, Pedro Alves
Comments
On 05/04/2017 07:22 PM, Pedro Alves wrote: > AFAICS so far, this is a false positive. > > Not sure what to do. I wouldn't want to force-memset > the optional's storage to work around it, which would be > a pessimization to quiet a warning. From above, we see that > that wouldn't work when we later start using std::optional. > > There's a bug open about this (for boost::optional, but most > probably the exact same): > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78044 I've tried the reproducer there with the obvious change to use std::optional and that does not warn. So this is a different, though related issue. Or maybe there's really something wrong with the gdb code that is escaping me. Thanks, Pedro Alves
On 05/04/2017 07:28 PM, Pedro Alves wrote: > On 05/04/2017 07:22 PM, Pedro Alves wrote: > >> AFAICS so far, this is a false positive. >> >> Not sure what to do. I wouldn't want to force-memset >> the optional's storage to work around it, which would be >> a pessimization to quiet a warning. From above, we see that >> that wouldn't work when we later start using std::optional. >> >> There's a bug open about this (for boost::optional, but most >> probably the exact same): >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78044 > > I've tried the reproducer there with the obvious change to use > std::optional and that does not warn. So this is a different, > though related issue. > > Or maybe there's really something wrong with the gdb code > that is escaping me. OK, here's smallest, self-contained reproducer I managed to come up with: ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ //#include <optional> #include <new> template<typename T> struct optional { optional () : m_dummy () {} ~optional () { m_item.~T (); } void emplace () { new (&m_item) T (); } union { int m_dummy; T m_item; }; }; template <typename T> using Optional = optional<T>; // warns //using Optional = std::optional<T>; // warns too extern int get (); extern void set (int); struct A { A () : m (get ()) {} ~A () { set (m); } int m; }; struct B { B (); ~B (); }; void func () { Optional<A> maybe_a; Optional<B> maybe_b; maybe_a.emplace (); maybe_b.emplace (); } ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ $ /opt/gcc/bin/g++ optional.cc -g3 -O2 -Wmaybe-uninitialized -std=gnu++17 -c optional.cc: In function ‘void func()’: optional.cc:28:15: warning: ‘maybe_a.optional<A>::<anonymous>.optional<A>::<unnamed union>::m_dummy’ may be used uninitialized in this function [-Wmaybe-uninitialized] ~A () { set (m); } ~~~~^~~ optional.cc:41:15: note: ‘maybe_a.optional<A>::<anonymous>.optional<A>::<unnamed union>::m_dummy’ was declared here Optional<A> maybe_a; ^~~~~~~ Looks like a compiler bug to me. Thanks, Pedro Alves
On 05/04/2017 10:49 PM, Pedro Alves wrote: > $ /opt/gcc/bin/g++ optional.cc -g3 -O2 -Wmaybe-uninitialized -std=gnu++17 -c > optional.cc: In function ‘void func()’: > optional.cc:28:15: warning: > ‘maybe_a.optional<A>::<anonymous>.optional<A>::<unnamed union>::m_dummy’ > may be used uninitialized in this function [-Wmaybe-uninitialized] > ~A () { set (m); } > ~~~~^~~ > optional.cc:41:15: note: > ‘maybe_a.optional<A>::<anonymous>.optional<A>::<unnamed union>::m_dummy’ > was declared here > Optional<A> maybe_a; > ^~~~~~~ > > Looks like a compiler bug to me. > Now reported: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80635 Thanks, Pedro Alves
diff --git c/gdb/infrun.c w/gdb/infrun.c index d0504de..11196ee 100644 --- c/gdb/infrun.c +++ w/gdb/infrun.c @@ -910,6 +910,8 @@ private: scoped_restore_current_inferior m_inferior; }; +#include <optional> + /* Called whenever we notice an exec or exit event, to handle detaching or resuming a vfork parent. */ @@ -936,9 +938,9 @@ handle_vfork_child_exec_or_exit (int exec) inf->vfork_parent->pending_detach = 0; - gdb::optional<scoped_restore_exited_inferior> + std::optional<scoped_restore_exited_inferior> maybe_restore_inferior; - gdb::optional<scoped_restore_current_pspace_and_thread> + std::optional<scoped_restore_current_pspace_and_thread> maybe_restore_thread; /* If we're handling a child exit, then inferior_ptid points