From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12830 invoked by alias); 14 May 2012 13:55:23 -0000 Received: (qmail 12810 invoked by uid 22791); 14 May 2012 13:55:21 -0000 X-SWARE-Spam-Status: No, hits=-3.7 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,SPF_HELO_PASS,TW_BJ,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from plane.gmane.org (HELO plane.gmane.org) (80.91.229.3) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 14 May 2012 13:55:06 +0000 Received: from list by plane.gmane.org with local (Exim 4.69) (envelope-from ) id 1STvjs-0004Kd-BL for gdb-patches@sources.redhat.com; Mon, 14 May 2012 15:55:04 +0200 Received: from ppp121-45-145-2.lns10.adl6.internode.on.net ([121.45.145.2]) by main.gmane.org with esmtp (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Mon, 14 May 2012 15:55:04 +0200 Received: from toojays by ppp121-45-145-2.lns10.adl6.internode.on.net with local (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Mon, 14 May 2012 15:55:04 +0200 To: gdb-patches@sources.redhat.com From: John Steele Scott Subject: Re: [patch] PR symtab/13277: Resolving opaque structures in ICC generated binaries. Date: Mon, 14 May 2012 13:55:00 -0000 Message-ID: <4FB10DD8.7040501@toojays.net> References: <4E9A6F3C.6010400@toojays.net> <20111019084011.GA9326@host1.jankratochvil.net> <4EA3E995.8040206@toojays.net> <20111026221057.GA24628@host1.jankratochvil.net> <4EBFB451.8030503@toojays.net> <4FA4912E.9050709@toojays.net> <20120512183722.GA20606@host2.jankratochvil.net> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: Tom Tromey User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.28) Gecko/20120313 Thunderbird/3.1.20 In-Reply-To: <20120512183722.GA20606@host2.jankratochvil.net> X-IsSubscribed: yes 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 X-SW-Source: 2012-05/txt/msg00504.txt.bz2 Jan, Thanks for your review. I have a few questions before I resubmit. On 13/05/12 04:07, Jan Kratochvil wrote: > On Sat, 05 May 2012 04:32:14 +0200, John Steele Scott wrote: >> I ran the dwarf2 and base tests and saw no new failures. Please apply. > > BTW did you run the testsuite with icc or gcc or both? So far I have only run the testsuite with gcc. Until now I've only been occasionally accessing icc on our build machine, which is not setup for gdb development. I may be able to get ICC setup on my local machine and try to build/test with that. Are the existing tests already known to work with ICC? >> --- a/gdb/dwarf2read.c >> +++ b/gdb/dwarf2read.c >> @@ -8727,6 +8727,23 @@ quirk_gcc_member_function_pointer (struct type *type, struct objfile *objfile) >> smash_to_methodptr_type (type, new_type); >> } >> >> +/* Return non-zero if the supplied PRODUCER string matches the Intel C/C++ >> + compiler (icc). */ >> + >> +static int >> +producer_is_icc (const char *producer) >> +{ >> + static const char *const icc_ident = "Intel(R) C Intel(R) 64 Compiler XE"; > > I believe the same problem applies to 32-bit compiler while this string will > not match it. Also isn't the same problem for example with some Intel Fortran > compiler? That maybe some "Intel(R)" match would be enough. You are correct about the 32-bit compiler. I don't know about the Fortran, we don't have that here. Just matching "Intel(R)" seems a little loose; wouldn't it be safer to try against each producer string in turn? I have added a check for "Intel(R) C Compiler XE" which is what the 32-bit version emits. If you know the Fortran producer string I can add that, or if you really prefer we can just scan for "Intel(R)" like you said before. >> + >> + if (producer == NULL) >> + return 0; >> + >> + if (strncmp (producer, icc_ident, strlen (icc_ident)) == 0) >> + return 1; >> + >> + return 0; >> +} >> + >> /* Called when we find the DIE that starts a structure or union scope >> (definition) to create a type for the structure or union. Fill in >> the type's name and general properties; the members will not be >> @@ -8837,6 +8854,11 @@ read_structure_type (struct die_info *die, struct dwarf2_cu *cu) >> /* RealView does not output the required DW_AT_declaration >> on incomplete types. */ >> TYPE_STUB (type) = 1; >> + else if (attr != NULL && die->child == NULL && TYPE_LENGTH (type) == 0 >> + && producer_is_icc (cu->producer)) >> + /* ICC does not output the required DW_AT_declaration >> + on incomplete types, but gives them a size of zero. */ >> + TYPE_STUB (type) = 1; > > braces; it is just said so in gdb/doc/gdbint.texinfo: > Any two or more lines in code should be wrapped in braces, even if > they are comments, as they look like separate statements: > > { > /* ICC does not output the required DW_AT_declaration > on incomplete types, but gives them a size of zero. */ > TYPE_STUB (type) = 1; > } > > >> >> /* We need to add the type field to the die immediately so we don't >> infinitely recurse when dealing with pointers to the structure >> @@ -8849,6 +8871,30 @@ read_structure_type (struct die_info *die, struct dwarf2_cu *cu) >> return type; >> } >> >> +/* Return non-zero if the DIE from the compilation unit CU is an incomplete >> + type. "An incomplete structure, union or class type is represented by a >> + structure, union or class entry that does not have a byte size attribute and >> + that has a DW_AT_declaration attribute." */ >> + >> +static int >> +die_is_incomplete_type (struct die_info *die, struct dwarf2_cu *cu) >> +{ >> + struct attribute *attr = dwarf2_attr (die, DW_AT_byte_size, cu); >> + >> + if (dwarf2_flag_true_p (die, DW_AT_declaration, cu) && attr == NULL) > > This conditional should use die_is_declaration like before, as you no longer > check DW_AT_specification == NULL, as die_is_declaration does check. > > But in general I do not understand why did not you put the ICC exception to > existing die_is_declaration instead. Even with your patch still various parts > of code call die_is_declaration which may be a problem for ICC. I do not have > a countercase at hand. I would try to patch die_is_declaration and see if it > will make any difference with icc on the testsuite, I do not have icc ready > now to make this simple test. Yes, since I don't have a deep understanding of either the dwarf standard nor gdb internals, I was being conservative and making the minimum change required for my test. I can merge this die_is_incomplete_type logic into die_is_declaration, but then it means there is no way to distinguish between a "declaration", and an "incomplete type". It seems okay (dwarf2 tests pass at least), as long as there no obscure corner of dwarf where this distinction matters to us. > >> + return 1; >> + else if (producer_is_icc (cu->producer) >> + && attr != NULL && DW_UNSND (attr) == 0 && die->child == NULL >> + && (die->tag == DW_TAG_structure_type >> + || die->tag == DW_TAG_class_type >> + || die->tag == DW_TAG_union_type)) >> + /* ICC does not output the required DW_AT_declaration on incomplete >> + structure, union and class types, but gives them a size of zero. */ >> + return 1; > > braces: > { > /* ICC does not output the required DW_AT_declaration on incomplete > structure, union and class types, but gives them a size of zero. */ > return 1; > } In this case, should I also add braces to the "else if" block just above, while I'm there? Or do you prefer to avoid the churn? If I move this into die_is_declaration I will move the above realview check there too, then I will add the braces anyway in that case. >> + >> + return 0; >> +} >> + >> /* Finish creating a structure or union type, including filling in >> its members and creating a symbol for it. */ >> >> @@ -9053,11 +9099,7 @@ process_structure_scope (struct die_info *die, struct dwarf2_cu *cu) >> child_die = sibling_die (child_die); >> } >> >> - /* Do not consider external references. According to the DWARF standard, >> - these DIEs are identified by the fact that they have no byte_size >> - attribute, and a declaration attribute. */ >> - if (dwarf2_attr (die, DW_AT_byte_size, cu) != NULL >> - || !die_is_declaration (die, cu)) >> + if (!die_is_incomplete_type (die, cu)) >> new_symbol (die, type, cu); So if I push the die_is_incomplete_type logic into die_is_declaration, the original check for the DW_AT_byte_size attribute above must be removed, otherwise my bug is not fixed. >> } >> >> @@ -10991,8 +11033,23 @@ read_partial_die (const struct die_reader_specs *reader, >> part_die->sibling = buffer + dwarf2_get_ref_die_offset (&attr).sect_off; >> break; >> case DW_AT_byte_size: >> - part_die->has_byte_size = 1; >> - break; >> + if (DW_UNSND (&attr) == 0 && producer_is_icc (cu->producer) >> + && (part_die->tag == DW_TAG_structure_type >> + || part_die->tag == DW_TAG_union_type >> + || part_die->tag == DW_TAG_class_type)) >> + { >> + /* ICC ddoes not output DW_AT_declaration on incomplete types, >> + instead giving them a size of zero. Fix that up so that we > > Two spaces after dot. > >> + treat this as an incomplete type. Ideally we would do this in > > Again. > >> + fixup_partial_die(), but that would mean re-reading the > > Remove those "()" for GNU Coding Standards compliance. > >> + DW_AT_byte_size attribute. */ >> + part_die->is_declaration = 1; >> + } >> + else >> + { >> + part_die->has_byte_size = 1; >> + } > > Needless braces but I do not mind. > >> + break; >> case DW_AT_calling_convention: >> /* DWARF doesn't provide a way to identify a program's source-level >> entry point. DW_AT_calling_convention attributes are only meant cheers, John