From patchwork Tue Apr 25 01:10:37 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 20127 Received: (qmail 33306 invoked by alias); 25 Apr 2017 01:10:45 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 33268 invoked by uid 89); 25 Apr 2017 01:10:42 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.7 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, RCVD_IN_SORBS_SPAM autolearn=ham version=3.3.2 spammy= X-HELO: mail-wr0-f177.google.com Received: from mail-wr0-f177.google.com (HELO mail-wr0-f177.google.com) (209.85.128.177) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 25 Apr 2017 01:10:39 +0000 Received: by mail-wr0-f177.google.com with SMTP id z52so59096565wrc.2 for ; Mon, 24 Apr 2017 18:10:41 -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=CTzxr6gcoJIaJaxsMStLJjHbKJC/qWHNGBFbaMNjj38=; b=aAMTS/0sIW9VHKqYXrVYpwrK4CAV/Br6xOHu0Fg7DHXNjKzDoaTC0MHuE42qElAZih GwwRianeCvAE0cuSjpzpCd7jdOWHvy2xJK/ls2dse0aDdOcbqCvZ+RiRrZFuD8Kky7sR m4jTowyG2tsZnioo/3oK4O96skdqtdST08xAUSxVfB1S0h1J5QF9aiywQGW97mFWt0s/ 8ZkEfwDWUruUU+zollvowFopG9TdcDk7eTrNQgB1vPPWfROK/TkXWHipuk2lyjqY4yV9 bmcuigepMuFye5DDSwSMCz4xAKyR3B3RhQ8j7gyTA2Qus9jrYaf5hHDZtl5xFBbihal0 iM2g== X-Gm-Message-State: AN3rC/71GMPl+wpn0yEtGgsUSBmDhLz0z9PbFgsnh2MJfjj4lPJQ1frS 3B+0uZwE3yvQfEBgkvSSow== X-Received: by 10.223.183.6 with SMTP id l6mr8565566wre.42.1493082639570; Mon, 24 Apr 2017 18:10:39 -0700 (PDT) Received: from [192.168.0.102] ([37.189.166.198]) by smtp.gmail.com with ESMTPSA id x70sm1813599wmf.1.2017.04.24.18.10.37 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 24 Apr 2017 18:10:38 -0700 (PDT) Subject: Re: [PATCH 2/5] Don't memcpy non-trivially-copyable types: Make enum_flags triv. copyable To: Simon Marchi References: <1492050475-9238-1-git-send-email-palves@redhat.com> <1492050475-9238-3-git-send-email-palves@redhat.com> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: Date: Tue, 25 Apr 2017 02:10:37 +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: On 04/20/2017 04:34 AM, Simon Marchi wrote: > On 2017-04-12 22:27, Pedro Alves wrote: >> + /* Define copy/move ctor/op= as defaulted so that enum_flags is >> + trivially copyable. */ >> + enum_flags (const enum_flags &other) = default; >> + enum_flags (enum_flags &&) noexcept = default; >> + enum_flags &operator= (const enum_flags &other) = default; >> + enum_flags &operator= (enum_flags &&) = default; > > What's the difference between defining these as default, and not > defining them at all (which I assume will still make the compiler emit > the default versions)? Yeah, there's no difference. I'll just remove them, since it clearly confuses things otherwise. Thanks. > Do you want to add a static_assert(is_pod...) for that class? For now > we have the VECs that will warn us if it becomes non-POD, but eventually > they'll be gone. I do, but enum_flags is a template, and we need to add the assertion to some template instantiation. I'd like to add it in the enum flags unit tests file, currently here [1]: https://github.com/palves/gdb/blob/palves/cxx11-enum-flags/gdb/enum-flags-selftests.c which I need to rebase and repost. I should be reposting that soon. [1] - I'll move it to unittests/ Thanks, Pedro Alves From 23bcc18f470ee4364bd362a8b78c6c1415a9dadb Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Tue, 25 Apr 2017 01:27:42 +0100 Subject: [PATCH] Don't memcpy non-trivially-copyable types: Make enum_flags triv. copyable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The delete-memcpy-with-non-trivial-types patch exposed many instances of this problem: src/gdb/btrace.h: In function ‘btrace_insn_s* VEC_btrace_insn_s_quick_insert(VEC_btrace_insn_s*, unsigned int, const btrace_insn_s*, const char*, unsigned int)’: src/gdb/common/vec.h:948:62: error: use of deleted function ‘void* memmove(T*, const U*, size_t) [with T = btrace_insn; U = btrace_insn; = void; size_t = long unsigned int]’ memmove (slot_ + 1, slot_, (vec_->num++ - ix_) * sizeof (T)); \ ^ src/gdb/common/vec.h:436:1: note: in expansion of macro ‘DEF_VEC_FUNC_O’ DEF_VEC_FUNC_O(T) \ ^ src/gdb/btrace.h:84:1: note: in expansion of macro ‘DEF_VEC_O’ DEF_VEC_O (btrace_insn_s); ^ [...] src/gdb/common/vec.h:1060:31: error: use of deleted function ‘void* memcpy(T*, const U*, size_t) [with T = btrace_insn; U = btrace_insn; = void; size_t = long unsigned int]’ sizeof (T) * vec2_->num); \ ^ src/gdb/common/vec.h:437:1: note: in expansion of macro ‘DEF_VEC_ALLOC_FUNC_O’ DEF_VEC_ALLOC_FUNC_O(T) \ ^ src/gdb/btrace.h:84:1: note: in expansion of macro ‘DEF_VEC_O’ DEF_VEC_O (btrace_insn_s); ^ So, VECs (given it's C roots) rely on memcpy/memcpy of VEC elements to be well defined, in order to grow/reallocate its internal elements array. This means that we can only put trivially copyable types in VECs. E.g., if a type requires using a custom copy/move ctor to relocate, then we can't put it in a VEC (so we use std::vector instead). But, as shown above, we're violating that requirement. btrace_insn is currently not trivially copyable, because it contains an enum_flags field, and that is itself not trivially copyable. This patch corrects that, by simply removing the user-provided copy constructor and assignment operator. The compiler-generated versions work just fine. Note that std::vector relies on std::is_trivially_copyable too to know whether it can reallocate its elements with memcpy/memmove instead of having to call copy/move ctors and dtors, so if we have types in std::vectors that weren't trivially copyable because of enum_flags, this will make such vectors more efficient. gdb/ChangeLog: 2017-04-25 Pedro Alves * common/enum-flags.h (enum_flags): Don't implement copy ctor and assignment operator. --- gdb/ChangeLog | 5 +++++ gdb/common/enum-flags.h | 10 ---------- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 33dd53b..f5419db 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,8 @@ +2017-04-25 Pedro Alves + + * common/enum-flags.h (enum_flags): Don't implement copy ctor and + assignment operator. + 2017-04-24 Yao Qi * doublest.c (convert_doublest_to_floatformat): Call diff --git a/gdb/common/enum-flags.h b/gdb/common/enum-flags.h index e63c8a4..ddfcddf 100644 --- a/gdb/common/enum-flags.h +++ b/gdb/common/enum-flags.h @@ -120,16 +120,6 @@ public: : m_enum_value ((enum_type) 0) {} - enum_flags (const enum_flags &other) - : m_enum_value (other.m_enum_value) - {} - - enum_flags &operator= (const enum_flags &other) - { - m_enum_value = other.m_enum_value; - return *this; - } - /* If you get an error saying these two overloads are ambiguous, then you tried to mix values of different enum types. */ enum_flags (enum_type e)