From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ot1-x32c.google.com (mail-ot1-x32c.google.com [IPv6:2607:f8b0:4864:20::32c]) by sourceware.org (Postfix) with ESMTPS id 2AC4C398841A for ; Wed, 16 Jun 2021 17:21:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 2AC4C398841A Received: by mail-ot1-x32c.google.com with SMTP id b18-20020a0568301052b0290449ba7eff3cso1453005otp.7 for ; Wed, 16 Jun 2021 10:21:51 -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:from:message-id:date :user-agent:mime-version:in-reply-to:content-language; bh=Qh9NYMschazMZ6+LPqwGHJE8m5Q/DClDBn5X+NdoxYg=; b=MvnXKp6j5888jSAwgJvGif5Y8LAqQmhsHBkIASJOMtxJmDnooCG9tdD1mGl8wWCIeH 1n4Gyrk7ZHkAzetA9/veJU+noJsgScgBW3ZqPFo1ftHf1l1GzKkY+UPdvkKl3uE9nWUO 57yIzpjWt0y8O49FjRZMOZwgaUJ2ssHWXfTfMv9MX1t2vjvR53hG7SH0NxM7hSkreIgL LlLJJxYunvecUP+axMPgj82c9pub4+eMlExrf0nlLjtWbwxIKu1SBFsIVcXqkhqK0k2T qDSTWepisD7x6UNSO/AxTMSnBQNm5THpwV2FFZzXH+mrzGhvZjTIKOUhm1Ec1HFl3Udr wg3Q== X-Gm-Message-State: AOAM532B/xR5xVoqzN9l565CHWJsLn/dbBXll3PMoAe1W/fh0fUtIfOB UPj6i9EC16wrtab0ELPh7cnbwQlo7aM= X-Google-Smtp-Source: ABdhPJxkhWhDzEDH1a5cQJSalz92tvfHMjZNpB/5XDRtXiFyWSod18yPoHKjz33LUwCh825ZzcSSVA== X-Received: by 2002:a9d:62ce:: with SMTP id z14mr841579otk.255.1623864110537; Wed, 16 Jun 2021 10:21:50 -0700 (PDT) Received: from [192.168.0.41] (97-118-105-195.hlrn.qwest.net. [97.118.105.195]) by smtp.gmail.com with ESMTPSA id m10sm559896oig.9.2021.06.16.10.21.49 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 16 Jun 2021 10:21:49 -0700 (PDT) Subject: Re: [PATCH] make rich_location safe to copy To: Jason Merrill , David Malcolm , gcc-patches References: <88448f63-87ad-c3a5-d38b-c94dd825e8d2@gmail.com> <0b9651cb91da83738060095f6adecd7f02392e55.camel@redhat.com> <0eb421c1-4a84-e776-8be7-9887aab36e81@gmail.com> <558c61be-f93f-75e9-3a14-d2d7d79bc751@gmail.com> <7e656451-95e4-5ebd-4988-1eb2d539983f@redhat.com> From: Martin Sebor Message-ID: <8471bbb6-df84-5afc-eee2-70f844ff1df0@gmail.com> Date: Wed, 16 Jun 2021 11:21:48 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: <7e656451-95e4-5ebd-4988-1eb2d539983f@redhat.com> Content-Type: multipart/mixed; boundary="------------6DAB42A555A5A9F48F6B5B27" Content-Language: en-US X-Spam-Status: No, score=-10.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 16 Jun 2021 17:21:52 -0000 This is a multi-part message in MIME format. --------------6DAB42A555A5A9F48F6B5B27 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit On 6/16/21 10:35 AM, Jason Merrill wrote: > On 6/16/21 12:11 PM, Martin Sebor wrote: >> On 6/16/21 9:06 AM, David Malcolm wrote: >>> On Wed, 2021-06-16 at 08:52 -0600, Martin Sebor wrote: >>>> On 6/16/21 6:38 AM, David Malcolm wrote: >>>>> On Tue, 2021-06-15 at 19:48 -0600, Martin Sebor wrote: >>>>> >>>>> Thanks for writing the patch. >>>>> >>>>>> While debugging locations I noticed the semi_embedded_vec template >>>>>> in line-map.h doesn't declare a copy ctor or copy assignment, but >>>>>> is being copied in a couple of places in the C++ parser (via >>>>>> gcc_rich_location).  It gets away with it most likely because it >>>>>> never grows beyond the embedded buffer. >>>>> >>>>> Where are these places?  I wasn't aware of this. >>>> >>>> They're in the attached file along with the diff to reproduce >>>> the errors. >>> >>> Thanks. >>> >>> Looks like: >>> >>>     gcc_rich_location richloc = tok->location; >>> >>> is implicitly constructing a gcc_rich_location, then copying it to >>> richloc.  This should instead be simply: >>> >>>     gcc_rich_location richloc (tok->location); >>> >>> which directly constructs the richloc in place, as I understand it. >> >> I see, tok->location is location_t here, and the gcc_rich_location >> ctor that takes it is not declared explicit (that would prevent >> the implicit conversion). >> >> The attached patch solves the rich_location problem by a) making >> the ctor explicit, b) disabling the rich_location copy ctor, c) >> changing the parser to use direct initialization.  (I CC Jason >> as a heads up on the C++ FE bits.) > > The C++ bits are fine. > >> The semi_embedded_vec should be fixed as well, regardless of this >> particular use and patch.  Let me know if it's okay to commit (I'm >> not open to disabling its copy ctor). > >> +  /* Not copyable or assignable.  */ > > This comment needs a rationale. Done in the attached patch. Providing a rationale in each instance sounds like a good addition to the coding conventions. Let me propose a patch for that. Martin --------------6DAB42A555A5A9F48F6B5B27 Content-Type: text/x-patch; charset=UTF-8; name="gcc-rich-location.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="gcc-rich-location.diff" gcc/cp/ChangeLog: * parser.c (cp_parser_selection_statement): Use direct initialization instead of copy. gcc/ChangeLog: * gcc-rich-location.h (gcc_rich_location): Make ctor explicit. libcpp/ChangeLog: * include/line-map.h (class rich_location): Disable copying and assignment. diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c index d57ddc4560d..848e4823258 100644 --- a/gcc/cp/parser.c +++ b/gcc/cp/parser.c @@ -12355,7 +12355,7 @@ cp_parser_selection_statement (cp_parser* parser, bool *if_p, IF_STMT_CONSTEVAL_P (statement) = true; condition = finish_if_stmt_cond (boolean_false_node, statement); - gcc_rich_location richloc = tok->location; + gcc_rich_location richloc (tok->location); bool non_compound_stmt_p = false; if (cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_BRACE)) { @@ -12383,7 +12383,7 @@ cp_parser_selection_statement (cp_parser* parser, bool *if_p, RID_ELSE)) { cp_token *else_tok = cp_lexer_peek_token (parser->lexer); - gcc_rich_location else_richloc = else_tok->location; + gcc_rich_location else_richloc (else_tok->location); guard_tinfo = get_token_indent_info (else_tok); /* Consume the `else' keyword. */ cp_lexer_consume_token (parser->lexer); diff --git a/gcc/gcc-rich-location.h b/gcc/gcc-rich-location.h index 00747631025..2a9e5db65d5 100644 --- a/gcc/gcc-rich-location.h +++ b/gcc/gcc-rich-location.h @@ -21,14 +21,16 @@ along with GCC; see the file COPYING3. If not see #define GCC_RICH_LOCATION_H /* A gcc_rich_location is libcpp's rich_location with additional - helper methods for working with gcc's types. */ + helper methods for working with gcc's types. The class is not + copyable or assignable because rich_location isn't. */ + class gcc_rich_location : public rich_location { public: /* Constructors. */ /* Constructing from a location. */ - gcc_rich_location (location_t loc, const range_label *label = NULL) + explicit gcc_rich_location (location_t loc, const range_label *label = NULL) : rich_location (line_table, loc, label) { } diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h index 7d964172469..464494bfb5b 100644 --- a/libcpp/include/line-map.h +++ b/libcpp/include/line-map.h @@ -1670,6 +1670,12 @@ class rich_location /* Destructor. */ ~rich_location (); + /* The class manages the memory pointed to by the elements of + the M_FIXIT_HINTS vector and is not meant to be copied or + assigned. */ + rich_location (const rich_location &) = delete; + void operator= (const rich_location &) = delete; + /* Accessors. */ location_t get_loc () const { return get_loc (0); } location_t get_loc (unsigned int idx) const; --------------6DAB42A555A5A9F48F6B5B27--