From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0b-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com [148.163.158.5]) by sourceware.org (Postfix) with ESMTPS id 371B53858D38 for ; Fri, 14 Oct 2022 23:24:02 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 371B53858D38 Received: from pps.filterd (m0098417.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.17.1.5/8.17.1.5) with ESMTP id 29ELSgf1012480 for ; Fri, 14 Oct 2022 23:24:01 GMT Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3k7fqhtd2m-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Fri, 14 Oct 2022 23:24:01 +0000 Received: from m0098417.ppops.net (m0098417.ppops.net [127.0.0.1]) by pps.reinject (8.17.1.5/8.17.1.5) with ESMTP id 29ENO1GK030553 for ; Fri, 14 Oct 2022 23:24:01 GMT Received: from ppma02wdc.us.ibm.com (aa.5b.37a9.ip4.static.sl-reverse.com [169.55.91.170]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3k7fqhtd2c-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 14 Oct 2022 23:24:01 +0000 Received: from pps.filterd (ppma02wdc.us.ibm.com [127.0.0.1]) by ppma02wdc.us.ibm.com (8.16.1.2/8.16.1.2) with SMTP id 29ENLVhP022062; Fri, 14 Oct 2022 23:24:00 GMT Received: from b03cxnp07027.gho.boulder.ibm.com (b03cxnp07027.gho.boulder.ibm.com [9.17.130.14]) by ppma02wdc.us.ibm.com with ESMTP id 3k30uasaga-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 14 Oct 2022 23:24:00 +0000 Received: from smtpav06.dal12v.mail.ibm.com ([9.208.128.130]) by b03cxnp07027.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 29ENNxwD6226540 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Fri, 14 Oct 2022 23:23:59 GMT Received: from smtpav06.dal12v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 62B6358055; Fri, 14 Oct 2022 23:23:59 +0000 (GMT) Received: from smtpav06.dal12v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id EBFF35805D; Fri, 14 Oct 2022 23:23:58 +0000 (GMT) Received: from li-e362e14c-2378-11b2-a85c-87d605f3c641.ibm.com (unknown [9.211.71.228]) by smtpav06.dal12v.mail.ibm.com (Postfix) with ESMTP; Fri, 14 Oct 2022 23:23:58 +0000 (GMT) Message-ID: <41e8f94ed30fb22c94a05a0823de2f5ae2250334.camel@us.ibm.com> From: Carl Love To: Kevin Buettner , gdb-patches@sourceware.org Cc: Ulrich Weigand , cel@us.ibm.com Date: Fri, 14 Oct 2022 16:23:58 -0700 In-Reply-To: <20221013194959.7164bb86@f35-zws-1> References: <28ce795ca489f69829207b2a7a535cf7f77f6dd8.camel@us.ibm.com> <8decd662134d57e8caf43960a1cdc47723e2bfe3.camel@us.ibm.com> <79d82ed277308ed5ce312bff398e770ab234390a.camel@us.ibm.com> <63f21a897f452d81a73fb386cb99110a359ef0b7.camel@de.ibm.com> <97275f61ef101a12cde8e5a45008ed8e479424eb.camel@us.ibm.com> <191f5826b228a7614c084c9704b086851d418c78.camel@us.ibm.com> <5405a79ecd6ed34646ad77eed0779063ee222d37.camel@de.ibm.com> <1d55efe113cdcf96b812055ee45196a554c4ca78.camel@us.ibm.com> <20221013194959.7164bb86@f35-zws-1> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.28.5 (3.28.5-18.el8) Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 X-Proofpoint-GUID: GFOpRlzq6tHdRY9L-7unMnUK-TuO_ZcM X-Proofpoint-ORIG-GUID: 0Mdg-ipuhg7RfZu8rM-2h6c60J6JUW37 Subject: RE: [PATCH 2/2] PowerPC, fix support for printing the function return value for non-trivial values. X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.545,FMLib:17.11.122.1 definitions=2022-10-14_11,2022-10-14_01,2022-06-22_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 clxscore=1015 malwarescore=0 bulkscore=0 adultscore=0 spamscore=0 suspectscore=0 lowpriorityscore=0 priorityscore=1501 mlxlogscore=999 impostorscore=0 phishscore=0 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2209130000 definitions=main-2210140127 X-Spam-Status: No, score=-11.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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, 14 Oct 2022 23:24:04 -0000 On Thu, 2022-10-13 at 19:49 -0700, Kevin Buettner wrote: > Hi Carl, > > While running "git am" to apply your patch, I ran into the following > problems... > > 1) The patch didn't apply cleanly due to some changes in > gdb/dwarf2/loc.c. > It seemed to me that it was likely due to this commit: > > bd2b40ac129 Change GDB to use frame_info_ptr > > After checking out bd2b40ac129~, I was able to apply your patch > via git am, though I did see some whitespace complaints... Yes, mainline has moved a bit. I refreshed the patch. I made the change mentioned by Bruno c/frame_info */frame_info_ptr/. I was then able to get gdb to compile. > > 2) Whitespace complaints: > > Applying: PowerPC, fix support for printing the function return value > for non-trivial values. > ...patch:161: indent with spaces. > "gdbarch_dump: get_return_buf_addr = <%s>\n", > ...patch:162: indent with spaces. > host_address_to_string (gdbarch- > >get_return_buf_addr)); > ...patch:182: indent with spaces. > gdbarch_get_return_buf_addr_ftype > get_return_buf_addr) > warning: 3 lines add whitespace errors. > > (I substituted some long paths local to my machines w/ '...'.) Yes, I have seen the complaints about the white spaces. The whitespace complaints come from file gdb/gdbarch.c which is a generated file. As I assume you know, the file was regenerated by the gdb/gdbarch.py based on the changes to gdb/gdbarch-components.py. The regenerated file is set to read only. Not sure what I can do to rectify the white spaces? Any ideas? > > I already mentioned this last week, but... > > s/GDB ABI/gdbarch method/ > > I guess you could also refer to it as an API which you did later on > in your commit remarks. (What I really object to is calling it an > "ABI", which it definitely is not - that said it's easy to mis-type > API > as ABI, so I do understand this mistake.) Yea, I think referring to it as a new gdbarch method is probably better than GDB API/ABI. I have changed the GDB API/ABI references to gdbarch method. I will need to update that in the message to the GDB maintainers as well when I send the next version of the patch. > > > on entry to a function. The new gdb ABI attempts to use the > > DW_OP_entry > > s/The new/On PowerPC, the new/ > s/GDB ABI/gdbarch method/ > > > value for the DWARF entries, when exiting the function, to > > determine the > > Missing '_' between "DW_OP_entry" and "value". I.e. it's > "DW_OP_entry_value", > not "DW_OP_entry value". Fixed. > > > value of r3 on entry to the function. This requires the use of the > > -fvar-tracking compiler option to compile the user application thus > > generating the DW_OP_entry_value in the binary. The DW_OP_entries > > in the > > I don't think that "DW_OP_entries" is correct here. Maybe > "DW_OP_entry_value > entries"? > > binary file enables GDB can resolve the DW_TAG_call_sites. This > > new API > > Maybe instead of "enables GDB can resolve the DW_TAG_call_sites", say > "allow GDB to resolve the DW_TAG_call_site entries". > > > > is used to get the return buffer address, in the case of a function > > returning a nontrivial data type, on exit from the function. The > > GDB > > function should_stop checks to see if RETURN_BUF is non-zero. By > > default, > > RETURN_BUF will be set to zero by the new ABI call for all > > architectures > > s/ABI/API/ > > > but PowerPC. The get_return_value function will be used to obtain > > the > > return value on these architectures as is currently being done if > > RETURN_BUF is zero. On PowerPC, the new ABI will return a nonzero > > address > > in RETURN_BUF if the value can be determined. The value_at > > function uses > > the return buffer address to get the return value. So, I tried to address all of the above comments about ABI, "DW_OP_entries" etc. The paragraph now reads: This patch adds a new gdbarch method to allow PowerPC to access the value of r3 on entry to a function. On PowerPC, the new gdb arch method attempts to use the DW_OP_entry_value for the DWARF entries, when exiting the function, to determine the value of r3 on entry to the function. This requires the use of the -fvar-tracking compiler option to compile the user application thus generating the DW_OP_entry_value in the binary. The DW_OP_entry_value entries in the binary file allows GDB to resolve the DW_TAG_call_site entries. This new gdbarch method is used to get the return buffer address, in the case of a function returning a nontrivial data type, on exit from the function. The GDB function should_stop checks to see if RETURN_BUF is non-zero. By default, RETURN_BUF will be set to zero by the new gdbarch method call for all architectures except PowerPC. The get_return_value function will be used to obtain the return value on all other architectures as is currently being done if RETURN_BUF is zero. On PowerPC, the new gdbarch method will return a nonzero address in RETURN_BUF if the value can be determined. The value_at function uses the return buffer address to get the return value. Which I think covers all of the comments above. > > > > This patch fixes five testcase failures in gdb.cp/non-trivial- > > retval.exp. > > < snip > > > frame_info *cur_frame); > > #endif /* ARCH_UTILS_H */ > > diff --git a/gdb/dwarf2/loc.c b/gdb/dwarf2/loc.c > > index ad45d57a654..25a7499fe9b 100644 > > --- a/gdb/dwarf2/loc.c > > +++ b/gdb/dwarf2/loc.c > > @@ -1332,7 +1332,7 @@ static const struct lval_funcs > > entry_data_value_funcs = > > Function always returns non-NULL value. It throws > > NO_ENTRY_VALUE_ERROR if it > > cannot resolve the parameter for any reason. */ > > It appears that the doc above was moved to dwarf2/loc.h. I think it > might be > better to remove the above comment and replace it with "/* See > dwarf2/loc.h. */". > Fixed. > > > > -static struct value * > > +extern struct value * > > Remove 'extern ' here. Fixed. > > > if (execution_direction == EXEC_REVERSE) > > diff --git a/gdb/ppc-sysv-tdep.c b/gdb/ppc-sysv-tdep.c > > index 14effb93210..59145f9f166 100644 > > --- a/gdb/ppc-sysv-tdep.c > > +++ b/gdb/ppc-sysv-tdep.c > > @@ -28,6 +28,7 @@ > > #include "objfiles.h" > > #include "infcall.h" > > #include "dwarf2.h" > > +#include "dwarf2/loc.h" > > #include "target-float.h" > > #include > > > > @@ -2156,3 +2157,39 @@ ppc64_sysv_abi_return_value (struct gdbarch > > *gdbarch, struct value *function, > > return RETURN_VALUE_STRUCT_CONVENTION; > > } > > > > +CORE_ADDR ppc64_sysv_get_return_buf_addr (struct type *val_type, > > + frame_info *cur_frame) > > +{ > > + /* On PowerPC, the ABI specifies r3 is the address where an > > argument > > s/specifies/specifies that/ > > I'm not sure about this phrase: "where an argument and". If you > mean > to say that r3 is sometimes used for arguments (when it's not used > for > the address of the return value buffer), then that's true, but it's > not relevant here. > > So, I guess that I'm asking that this comment be reworded somewhat. > > > + and the return value is stored. The PowerPC ABI does not > > guarantee > > + r3 will be preserved when the function returns. The function > > returns > > + the address of the buffer stored in r3. > > I find this part confusing. If the function returns the address of > the buffer, doesn't it do so by returning it in r3? (And, if so, it > seems to me that this code would be unnecessary - so, clearly, I must > be missing something...) > > + > > + Attempt to determine the value of r3 on entry to the function > > using the > > + DW_OP_entry_value DWARF entries. This requires compiling the > > user > > + program with -fvar-tracking to resolve the DW_TAG_call_sites > > in the > > + binary file. */ OK, so some rework here to make things a bit better. I changed the header comment for function ppc64_sysv_get_return_buf_addr to the following. Hopefully this addresses the various questions and comments. /* The PowerPC ABI specifies aggregates that are not returned by value are returned in a storage buffer provided by the caller. The address of the storage buffer is provided as a hidden first input arguement in register r3. The PowerPC ABI does not guarantee that register r3 will not be changed while executing the function. Hence, it cannot be assumed that r3 will still contain the address of the storage buffer when execution reaches the end of the function. This function attempts to determine the value of r3 on entry to the function using the DW_OP_entry_value DWARF entries. This requires compiling the user program with -fvar-tracking to resolve the DW_TAG_call_sites in the binary file. */ Hopefully that is clearer and clears up any confusion as to the role of r3. > > + union call_site_parameter_u kind_u; > > + enum call_site_parameter_kind kind; > > + CORE_ADDR return_val = 0; > > + > > + kind_u.dwarf_reg = 3; /* First passed arg/return value is in > > r3. */ > > + kind = CALL_SITE_PARAMETER_DWARF_REG; > > + > > + /* val_type is the type of the return value. Need the pointer > > type > > + to the return value. */ > > + val_type = lookup_pointer_type (val_type); > > + > > + try > > + { > > + return_val = value_as_address (value_of_dwarf_reg_entry > > (val_type, > > + cur_fram > > e, > > + kind, > > kind_u)); > > + } > > + catch (const gdb_exception_error &e) > > + { > > + warning ("Cannot determine the function return value.\n" > > + "Try compiling with -fvar-tracking."); > > Will this warning be printed a lot? (I'm wondering if there should > be a > way to shut it off...) Hmm, hard to say how often it will get printed. It is going to be up to what the user is trying to do. If they are stopping on the return of functions that return non-trivial structures, they will see the message for every function. Otherwise, the message should not get printed. Not aware of anyway we could prompt the user to ask them if they want to suppress this message from the command line. If there is, can you give me a pointer to an example? We could do something like: catch (const gdb_exception_error &e) { static int warned = 0; if (warned++ < 6) warning ("Cannot determine the function return value.\n" "Try compiling with -fvar-tracking."); } So the warning is only printed the first five or whatever number of times we want before suppressing the message. Not sure what the right number of times should be? Would adding something like this be agreeable? Carl