From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 96572 invoked by alias); 24 Oct 2018 21:00:53 -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 96157 invoked by uid 89); 24 Oct 2018 21:00:27 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-0.9 required=5.0 tests=BAYES_00,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_PASS autolearn=no version=3.3.2 spammy=52, 5.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 24 Oct 2018 21:00:25 +0000 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E230AC0BF2BA; Wed, 24 Oct 2018 21:00:16 +0000 (UTC) Received: from theo.uglyboxes.com (ovpn04.gateway.prod.ext.phx2.redhat.com [10.5.9.4]) by smtp.corp.redhat.com (Postfix) with ESMTPS id A39F9662FB; Wed, 24 Oct 2018 21:00:16 +0000 (UTC) Subject: Re: [PATCH] gdb: Handle ICC's unexpected void return type To: Andrew Burgess , gdb-patches@sourceware.org References: <20181023212843.4774-1-andrew.burgess@embecosm.com> From: Keith Seitz Message-ID: <0794714d-b2f8-6175-cc98-6000d77ea933@redhat.com> Date: Wed, 24 Oct 2018 21:00:00 -0000 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: <20181023212843.4774-1-andrew.burgess@embecosm.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2018-10/txt/msg00566.txt.bz2 On 10/23/18 2:28 PM, Andrew Burgess wrote: > So function 'function_foo' has void return type, but still has a > DW_AT_type attribute for a 0 sized, type called void. Not only that, but the C/C++ "void" type is explicitly called out in the spec (section 5.2)... > gdb/ChangeLog: > > * dwarf2read.c (struct dwarf2_cu): Add producer_is_icc field. > (producer_is_icc): New function. > (check_producer): Set producer_is_icc field on dwarf2_cu. > (read_base_type): Spot ICC's unexpected void type, and convert to > GDB's expected void type. > (dwarf2_cu::dwarf2_cu): Initialise producer_is_icc field. > * valprint.c (maybe_negate_by_bytes): Add an assertion that the > LEN is greater than 0. While I think this is reasonable (and non-invasive/safe), I have some questions. > @@ -17548,7 +17565,11 @@ read_base_type (struct die_info *die, struct dwarf2_cu *cu) > type = dwarf2_init_float_type (objfile, bits, name, name); > break; > case DW_ATE_signed: > - type = init_integer_type (objfile, bits, 0, name); > + if (bits == 0 && producer_is_icc (cu) > + && strcmp (name, "void") == 0) > + type = objfile_type (objfile)->builtin_void; > + else > + type = init_integer_type (objfile, bits, 0, name); > break; > case DW_ATE_unsigned: > if (cu->language == language_fortran Is this the appropriate place for this? The patch is attempting to deal specifically with the void return of a function. I would have thought to catch this anomaly in read_func_scope/add_dwarf2_member_fn. These sorts of exceptions are handled relatively commonly in dwarf2read.c. While the DWARF specifications specifically call out using DW_TAG_unspecified_type for the void type (in C/C++), it doesn't actually say that this particular usage is invalid AFAICT. [Although I think it is.] Nonetheless, changing this here would preclude some language from doing something like this (as silly as that sounds). Also, if it this is the appropriate place (or even if it is decided to move this check elsewhere), why limit this to ICC? Is it simply because ICC only handles C/C++? Would it hurt/be worth it to safe guard that gcc or clang or rustc or who-knows-what wouldn't cause us similar harm? As for the actual code as it is, why only check DW_ATE_signed? I realize that this is specifically to address the particular ICC instance you are trying to fix, but something tells me that this could potentially change. Regardless of the encoding, this would be invalid. I'm not really asking for any changes. After looking at all kinds of DWARF output that I never dreamed possible, my thoughts tend to defensive programming in the DWARF reader. Thanks, Keith (IANAM*) * I Am Not A Maintainer