From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f66.google.com (mail-wr1-f66.google.com [209.85.221.66]) by sourceware.org (Postfix) with ESMTPS id 2DA4C38618DF for ; Fri, 28 Aug 2020 20:12:19 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 2DA4C38618DF Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=alves.ped@gmail.com Received: by mail-wr1-f66.google.com with SMTP id x7so284052wro.3 for ; Fri, 28 Aug 2020 13:12:19 -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 :content-transfer-encoding; bh=p+gGPp1LRReVpcZiLRMfaictW258D0AXWR2jdoBg+5s=; b=tedSx3E/jX0yH8+HER1sB1wr9vHwm5Ft3rYDoQYCVPaHuYPhu+HkLMhfcZeci+FZVy Ip1h4BtNmO+FTtPaqaT70hO1aVFtpBq7Em9cssU+N3QpBfP7i7T1VrMPo0sEGCDcAbxg xjOghKij4rh+FhdI9/BzAVtiDDOjF9oHH0U2O3LWB3njdcBg8uW72l1Rod46rJiI20aG +Bt+BpIAPnhKtVBoaIDccxwGDOg2uRcDUgBjOAjtTWM+iMu8bBtjMNBJeG3R8ROssKlM 9beDJO0r58J/XWOorkKdx0fvNmGVvu0ZbVfdhAhICdeg2b1gKVRqIDhB4IFaMpcSewBN G3Uw== X-Gm-Message-State: AOAM533q0IQ7zevpjZbhuEEMrDHXQNV0/FNAj6zWJujSkqHO+GU9IXTr Eks3YrxvuVY/9mrD6W9Qb10DN+J6e6nv4w== X-Google-Smtp-Source: ABdhPJynm9Oc4M/JgGUld3eXdoDMJPEynpYkMX3k+l/VPD9K0jj2XVcaw9UD8oLGzo3l/zmV+dTD7g== X-Received: by 2002:adf:f247:: with SMTP id b7mr622584wrp.128.1598645536932; Fri, 28 Aug 2020 13:12:16 -0700 (PDT) Received: from ?IPv6:2001:8a0:f905:5600:56ee:75ff:fe8d:232b? ([2001:8a0:f905:5600:56ee:75ff:fe8d:232b]) by smtp.gmail.com with ESMTPSA id y3sm814181wrs.36.2020.08.28.13.12.15 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 28 Aug 2020 13:12:15 -0700 (PDT) Subject: Re: [PATCH] Reject ambiguous C++ field accesses To: Luis Machado , gdb-patches@sourceware.org References: <20200827180251.20244-1-pedro@palves.net> From: Pedro Alves Message-ID: Date: Fri, 28 Aug 2020 21:12:14 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-7.5 required=5.0 tests=BAYES_00, BODY_8BITS, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, 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: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 28 Aug 2020 20:12:21 -0000 On 8/27/20 8:58 PM, Luis Machado wrote: >> diff --git a/gdb/testsuite/gdb.cp/ambiguous.exp b/gdb/testsuite/gdb.cp/ambiguous.exp >> index 17fb29f5350..74783cd0725 100644 >> --- a/gdb/testsuite/gdb.cp/ambiguous.exp >> +++ b/gdb/testsuite/gdb.cp/ambiguous.exp >> @@ -15,15 +15,9 @@ >>     # This file is part of the gdb testsuite >>   -# tests relating to ambiguous class members >> -# Written by Satish Pai 1997-07-28 >> - >> -# This file is part of the gdb testsuite >> - >> -# >> -# test running programs >> -# >> - >> +# Print out various class objects' members and check that the error >> +# about the field or baseclass being ambiguious is emitted at the >> +# right times. > > Typo, ambiguious. Thanks, fixed. > >>     if { [skip_cplus_tests] } { continue } >>   @@ -47,180 +41,225 @@ if {[prepare_for_testing "failed to prepare" $testfile $srcfile \ >>       return -1 >>   } >>   -# >> -# set it up at a breakpoint so we can play with the variable values >> -# >> +# Set it up at a breakpoint so we can play with the variable values. >> + > > This old comment reads funny. Set what up? I'd rephrase a bit. This comment is just before the starting up the inferior and running to a breakpoint. It's talking about that. Setting up the inferior ready for testing. I changed it to: # Run to a breakpoint after the variables have been initialized so we # can play with the variable values. But I guess I could just remove it altogether, since it's pretty obvious what we're doing. >> diff --git a/gdb/valops.c b/gdb/valops.c >> index 0eb2b096211..0e030a03ecd 100644 >> --- a/gdb/valops.c >> +++ b/gdb/valops.c >> @@ -1766,25 +1766,105 @@ typecmp (int staticp, int varargs, int nargs, >>     return i + 1; >>   } >>   -/* Helper class for do_search_struct_field that updates *RESULT_PTR >> -   and *LAST_BOFFSET, and possibly throws an exception if the field >> -   search has yielded ambiguous results.  */ >> +/* Helper class for search_struct_field that keeps track of found >> +   results.  See search_struct_field for description of >> +   LOOKING_FOR_BASECLASS.  If LOOKING_FOR_BASECLASS is true, possibly >> +   throws an exception if the base class search has yielded ambiguous > > "throw" an exception instead? I'll handle this one in response to Gary. > >> +   results.  If LOOKING_FOR_BASECLASS is false, found fields are >> +   accumulated and the caller (search_struct_field takes) care of > > "takes" outside the parenthesis. Fixed. > >> +   throwing an error if the field search yielded ambiguous results. >> +   The latter is done that way so that the error message can include a >> +   list of all the found candidates.  */ >> + >> +struct struct_field_searcher >> +{ >> +  /* A found field.  */ >> +  struct found_field >> +  { >> +    /* Patch to the structure where the field was found.  */ > > Typo... Path. :-) Fixed. >> +  /* Whether we're looking for a baseclass, or a field.  */ >> +  const bool m_looking_for_baseclass; >> + >> +  /* The offset of base class containing the field/baseclass we last >> +     recorded.  */ > > Maybe "offset into base class"? Thanks. However, it's the offset of the class itself, not the field. So I've changed it to: /* The offset of the baseclass containing the field/baseclass we last recorded. */ > Otherwise LGTM. I exercised this for aarch64-linux and see no more failures and 10 KFAIL's. Great, thanks!