From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qk1-x744.google.com (mail-qk1-x744.google.com [IPv6:2607:f8b0:4864:20::744]) by sourceware.org (Postfix) with ESMTPS id E2F8E3870879 for ; Thu, 27 Aug 2020 18:45:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org E2F8E3870879 Received: by mail-qk1-x744.google.com with SMTP id z3so6922292qkz.7 for ; Thu, 27 Aug 2020 11:45:42 -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=wNXdjo+RzMBoCp+FIbsWSHZ3LiBh4MDfwDbjL1Dqzms=; b=WD5OcSRtCCaePkHs4N46HBFrdwUwguTIMhLO3wcyDiKS++Twxw5WWjh9mA5GD/WLvv T/fVY1yru2wlwgpe5UG4GWPa2Vy4HeCEJ491ej8E0wYqHeFO101hnF7Tbq4aNmTXCWyg Gok2cV8LAOzZ/Bf1NTUmy5gjhI0/QvX2CT5Rwh10eP8QAZ7k2rVXPEUtRecym3bz7/9g aZPtEQSzHz5uEgfrrRdskxs5ny/M/9oWvOz0ySwRALsIF3BnekR7vJmoq+LCSSma9GzF JgF2lN+Zs3LtsQehJYd0f9IMMRZnnzZHcaFOwPF/uEgxabKJO+A68wvdIDQseOzSWv9v Entg== X-Gm-Message-State: AOAM530xkf8mNq6Cfc/JKAIbjLLlrC+PBfZZrmSqEWcujLRaCwmtvWnV OX14RC0uO8L1euXQPT+4NARjdQ2nMoS54g== X-Google-Smtp-Source: ABdhPJxix8011uEzmDHGD5ujehSmdq/jiiW0nQRkxsjFSWdeeuBbvpVLgtU0AgZ+3j1ac+X6DVRmWQ== X-Received: by 2002:a37:e50e:: with SMTP id e14mr205417qkg.323.1598553942094; Thu, 27 Aug 2020 11:45:42 -0700 (PDT) Received: from ?IPv6:2804:7f0:8283:a3ee:1908:8b5e:50f8:c881? ([2804:7f0:8283:a3ee:1908:8b5e:50f8:c881]) by smtp.gmail.com with ESMTPSA id x28sm2380155qki.55.2020.08.27.11.45.40 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 27 Aug 2020 11:45:41 -0700 (PDT) Subject: Re: [PATCH] Reject ambiguous C++ field accesses To: Pedro Alves , gdb-patches@sourceware.org References: <20200827180251.20244-1-pedro@palves.net> From: Luis Machado Message-ID: Date: Thu, 27 Aug 2020 15:45:37 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: <20200827180251.20244-1-pedro@palves.net> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-5.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, 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: 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: Thu, 27 Aug 2020 18:45:44 -0000 On 8/27/20 3:02 PM, Pedro Alves wrote: > The gdb.cp/ambiguous.exp testcase had been disabled for many years, > but recently it was re-enabled. However, it is failing everywhere. > That is because it is testing an old feature that is gone from GDB. > > The testcase is expecting to see an ambiguous field warning, like: > > # X is derived from A1 and A2; both A1 and A2 have a member 'x' > send_gdb "print x.x\n" > gdb_expect { > -re "warning: x ambiguous; using X::A2::x. Use a cast to disambiguate.\r\n\\$\[0-9\]* = \[-\]*\[0-9\]*\r\n$gdb_prompt $" { > pass "print x.x" > } > -re "warning: x ambiguous; using X::A1::x. Use a cast to disambiguate.\r\n\\$\[0-9\]* = \[-\]*\[0-9\]*\r\n$gdb_prompt $" { > pass "print x.x" > } > -re ".*$gdb_prompt $" { fail "print x.x" } > timeout { fail "(timeout) print x.x" } > } > > However, GDB just accesses one of the candidates without warning or > error: > > print x.x > $1 = 1431655296 > (gdb) FAIL: gdb.cp/ambiguous.exp: print x.x > > (The weird number is because the testcase does not initialize the > variables.) > > The testcase come in originally with the big HP merge: > > +Sun Jan 10 23:44:11 1999 David Taylor > + > + > + The following files are part of the HP merge; some had longer > + names at HP, but have been renamed to be no more than 14 > + characters in length. > > Looking at the tree back then, we find that warning: > > /* Helper function used by value_struct_elt to recurse through baseclasses. > Look for a field NAME in ARG1. Adjust the address of ARG1 by OFFSET bytes, > and search in it assuming it has (class) type TYPE. > If found, return value, else return NULL. > > If LOOKING_FOR_BASECLASS, then instead of looking for struct fields, > look for a baseclass named NAME. */ > > static value_ptr > search_struct_field (name, arg1, offset, type, looking_for_baseclass) > char *name; > register value_ptr arg1; > int offset; > register struct type *type; > int looking_for_baseclass; > { > int found = 0; > char found_class[1024]; > value_ptr v; > struct type *vbase = NULL; > > found_class[0] = '\000'; > > v = search_struct_field_aux (name, arg1, offset, type, looking_for_baseclass, &found, found_class, &vbase); > if (found > 1) > warning ("%s ambiguous; using %s::%s. Use a cast to disambiguate.", > name, found_class, name); > > return v; > } > > > However, in current GDB, search_struct_field does not handle the > ambiguous field case, nor is that warning found anywhere. Somehow it > got lost over the years. That seems like a regression, because the > compiler (as per language rules) rejects the ambiguous accesses as > well. E.g.: > > gdb.cp/ambiguous.cc:98:5: error: request for member 'x' is ambiguous > 98 | x.x = 1; > | ^ > gdb.cp/ambiguous.cc:10:7: note: candidates are: 'int A2::x' > 10 | int x; > | ^ > gdb.cp/ambiguous.cc:4:7: note: 'int A1::x' > 4 | int x; > | ^ > > > This patch restores the feature, though implemented differently and > with better user experience, IMHO. An ambiguous access is now an > error instead of a warning, and also GDB shows you the all candidates, > like: > > (gdb) print x.x > Request for member 'x' is ambiguous in type 'X'. Candidates are: > 'int A1::x' (X -> A1) > 'int A2::x' (X -> A2) > (gdb) print j.x > Request for member 'x' is ambiguous in type 'J'. Candidates are: > 'int A1::x' (J -> K -> A1) > 'int A1::x' (J -> L -> A1) Would it make sense to spare the users the touble of having to re-type/cast things and, instead, display all the possible values with an indication of what the fields are for each value? That would be more intuitive to me, given the debugger is only inspecting things and not enforcing a particular syntax for a source file. Users tend to be lazy. They just want to see values, not be syntactically correct. :-)