From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 90352 invoked by alias); 29 Oct 2019 22:13:08 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 90339 invoked by uid 89); 29 Oct 2019 22:13:08 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-8.7 required=5.0 tests=AWL,BAYES_00 autolearn=ham version=3.3.1 spammy=1408 X-HELO: mx1.osci.io Received: from polly.osci.io (HELO mx1.osci.io) (8.43.85.229) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 29 Oct 2019 22:13:06 +0000 Received: by mx1.osci.io (Postfix, from userid 994) id 0FD0220468; Tue, 29 Oct 2019 18:13:04 -0400 (EDT) Received: from gnutoolchain-gerrit.osci.io (gnutoolchain-gerrit.osci.io [8.43.85.239]) by mx1.osci.io (Postfix) with ESMTP id A6C7E20D23; Tue, 29 Oct 2019 18:13:02 -0400 (EDT) Received: from localhost (localhost [127.0.0.1]) by gnutoolchain-gerrit.osci.io (Postfix) with ESMTP id 7108420AF6; Tue, 29 Oct 2019 18:13:02 -0400 (EDT) X-Gerrit-PatchSet: 1 Date: Tue, 29 Oct 2019 22:13:00 -0000 From: "Tom Tromey (Code Review)" To: Tankut Baris Aktemur , gdb-patches@sourceware.org Cc: Andrew Burgess Auto-Submitted: auto-generated X-Gerrit-MessageType: comment Subject: [review] infcall, c++: collect more pass-by-reference information X-Gerrit-Change-Id: Ic05bd98a962d07ec3c1ad041f709687eabda3bb9 X-Gerrit-Change-Number: 137 X-Gerrit-ChangeURL: X-Gerrit-Commit: 3a1fe43a7b47d749f3bcef98d58983cc9cd26675 In-Reply-To: References: X-Gerrit-Comment-Date: Tue, 29 Oct 2019 18:13:02 -0400 Reply-To: gnutoolchain-gerrit@osci.io MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Disposition: inline User-Agent: Gerrit/3.0.3-74-g460fb0f7e9 Content-Type: text/plain; charset=UTF-8 Message-Id: <20191029221302.7108420AF6@gnutoolchain-gerrit.osci.io> X-SW-Source: 2019-10/txt/msg01073.txt.bz2 Tom Tromey has posted comments on this change. Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/137 ...................................................................... Patch Set 1: Code-Review+1 (3 comments) Thank you. I appreciate what you've done here (in particular the comment on the test case, but really all of it looks quite good). There were some nits in this one so I am marking it +1. https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/137/1/gdb/gnu-v3-abi.c File gdb/gnu-v3-abi.c: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/137/1/gdb/gnu-v3-abi.c@1321 PS1, Line 1321: 1296 | is_copy_or_move_constructor_type (struct type *class_type, | ... 1316 | then this is not a copy or move constructor, but just a 1317 | constructor. */ 1318 | for (int i = 2; i < TYPE_NFIELDS (method_type); i++) 1319 | { 1320 | arg_type = TYPE_FIELD_TYPE (method_type, i); 1321 | /* FIXME taktemur/2019-04-23: As of this date, neither 1322 | clang++-7.0.0 nor g++-8.2.0 produce a DW_AT_default_value 1323 | attribute. GDB is also not set to read this attribute, yet. 1324 | Hence, we immediately return false if there are more than 1325 | 2 parameters. */ It would be good to ensure that there is a gcc bug report for this, and then link to the bug from this comment. https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/137/1/gdb/gnu-v3-abi.c@1413 PS1, Line 1413: 1371 | gnuv3_pass_by_reference (struct type *type) | ... 1404 | has_cc_attr = true; 1405 | is_pass_by_value = false; 1406 | } 1407 | 1408 | /* A dynamic class has a non-trivial copy constructor. 1409 | See c++98 section 12.8 Copying class objects [class.copy]. */ 1410 | if (gnuv3_dynamic_class (type)) 1411 | is_dynamic = true; 1412 | 1413 | /* FIXME taktemur/2019-04-23: What if there are multiple copy ctors? I think it would be good to answer this before landing. Maybe we need to do overload resolution? https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/137/1/gdb/gnu-v3-abi.c@1495 PS1, Line 1495: 1371 | gnuv3_pass_by_reference (struct type *type) | ... 1486 | about recursive loops here, since we are only looking at members 1487 | of complete class type. Also ignore any static members. */ 1488 | for (fieldnum = 0; fieldnum < TYPE_NFIELDS (type); fieldnum++) 1489 | if (!field_is_static (&TYPE_FIELD (type, fieldnum))) 1490 | { 1491 | struct type *field_type = TYPE_FIELD_TYPE (type, fieldnum); 1492 | 1493 | /* For arrays, make the decision based on the element type. */ 1494 | if (TYPE_CODE (field_type) == TYPE_CODE_ARRAY) 1495 | field_type = field_type->main_type->target_type; I think it's more usual to use the TYPE_TARGET_TYPE accessor here. Also normally one must call check_typedef on the result. -- Gerrit-Project: binutils-gdb Gerrit-Branch: master Gerrit-Change-Id: Ic05bd98a962d07ec3c1ad041f709687eabda3bb9 Gerrit-Change-Number: 137 Gerrit-PatchSet: 1 Gerrit-Owner: Tankut Baris Aktemur Gerrit-Reviewer: Andrew Burgess Gerrit-Reviewer: Tankut Baris Aktemur Gerrit-Reviewer: Tom Tromey Gerrit-Comment-Date: Tue, 29 Oct 2019 22:13:02 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment