From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id E6DFC3858D32 for ; Mon, 26 Sep 2022 10:43:50 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org E6DFC3858D32 Received: from mail-ed1-f69.google.com (mail-ed1-f69.google.com [209.85.208.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-540-Xy9mASt_PMuwomp5kpGKWQ-1; Mon, 26 Sep 2022 06:43:46 -0400 X-MC-Unique: Xy9mASt_PMuwomp5kpGKWQ-1 Received: by mail-ed1-f69.google.com with SMTP id t13-20020a056402524d00b00452c6289448so5117924edd.17 for ; Mon, 26 Sep 2022 03:43:46 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date; bh=wXGUyf16zVy6G2tg7v2w7F9rKb58d7jFY58dJFqOvXs=; b=t6hsn4CBbR7A14M6WlyliUryC1q7M9ZYExQkC8vMBHtWrCMg9nBxFQgea4TDqlMEIz UNgZGK+Dg93ZVPwjkmtk/BNNM4Uur59Yyot6HuVonUgdrEWA4lXQsnfExDWYZZ6aMNaZ nRLkAFxaiO/UJJIXpsRr3tbpm0vJ7vniNic5jJItKeyLdaT4bEHa16zvzA4HWEPdaTqC 8jfsXtq3NC4Lq1Kv8wHuWMlpQYpJGyeF7N87zI0PjuQAASy+HsDAWaGoCVuXZSionVhG 3JqRYkCVUYiTgtVGs7M/wVssU68BpQfvAtPZRKyziYf5ICMYBha7rCfk0RQpz+qS31Zi IafQ== X-Gm-Message-State: ACrzQf0DoVpOd6JwPZNNvdEQOuLlRUtxkkYqI54RIuiQftJruTfi8Alv v5GzVUODsfShlM9FZsAOtlibs3VVN2WRGKh0TS0tHQ3OZRHCGytNBw5B4CCGtCUqtypwC69CqpF r5rw/krje4Kbl3664RHfSxw== X-Received: by 2002:a17:906:fe46:b0:73d:939a:ec99 with SMTP id wz6-20020a170906fe4600b0073d939aec99mr17814619ejb.169.1664189024952; Mon, 26 Sep 2022 03:43:44 -0700 (PDT) X-Google-Smtp-Source: AMsMyM63XmWWkWoAW/eXPYJAmU3egszsCfvt+409zMSV4z8nCvx+JsXrUGDCX0lGNg1T2VdrDBHPGg== X-Received: by 2002:a17:906:fe46:b0:73d:939a:ec99 with SMTP id wz6-20020a170906fe4600b0073d939aec99mr17814592ejb.169.1664189024508; Mon, 26 Sep 2022 03:43:44 -0700 (PDT) Received: from [192.168.0.45] (ip-213-220-232-121.bb.vodafone.cz. [213.220.232.121]) by smtp.gmail.com with ESMTPSA id fg4-20020a056402548400b0044f1fcf5ee0sm11044018edb.48.2022.09.26.03.43.43 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 26 Sep 2022 03:43:43 -0700 (PDT) Message-ID: Date: Mon, 26 Sep 2022 12:43:42 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.2.1 Subject: Re: [PATCH 2/3] [RFC][gdb/symtab] Add maint set symbol-read-order To: Tom de Vries , gdb-patches@sourceware.org References: <20220922152042.21914-1-tdevries@suse.de> <20220922152042.21914-2-tdevries@suse.de> From: Bruno Larsen In-Reply-To: <20220922152042.21914-2-tdevries@suse.de> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-13.6 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, 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: Mon, 26 Sep 2022 10:43:53 -0000 Cheers, Bruno On 22/09/2022 17:20, Tom de Vries via Gdb-patches wrote: > With the test-case included in this patch, we run into: > ... > (gdb) print var_dwarf^M > $1 = 1^M > (gdb) PASS: gdb.ctf/dwarf2-and-ctf.exp: print var_dwarf > print var_ctf^M > 'var_ctf' has unknown type; cast it to its declared type^M > (gdb) FAIL: gdb.ctf/dwarf2-and-ctf.exp: print var_ctf > ... > > The problem is that the executable contains both ctf and dwarf2, so the ctf > info is ignored. > > GDB has support for handling multiple debug formats, but the common use case > for ctf is to be used when dwarf2 is not present, and gdb reflects that, > assuming that by reading ctf in addition there won't be any extra information, > so it's not worth the additional cycles and memory. > > Add a new maintenance command "maint set symbol-read-order ", where > the argument string "mdebug+stabs+dwarf2/ctf" represent the current default, > in other words: > - first mdebug is read > - then stabs is read > - then dwarf2 is read, > - then ctf is read, but only if dwarf2 info was missing. > > A bit of background on the relevance of reading order: the formats that create > partial symtabs (all but dwarf2) have a priority relationship between them, > where reading earlier means lower priority. By reading the format with the > most detail last, we ensure it has the highest priority, which makes sure that > in case there is overlapping info, the most detailed info is found. This > explains the ordering of mdebug, stabs and dwarf2 (because it used to have > partial symtabs). > > By using "maint set symbol-read-order ctf+dwarf2 or dwarf2+ctf, we can make the > test-case pass. > > This is an RFC at this point, so no documentation yet. > > Points I'd like to have feedback on: > - I chose this to be a maintenance command, because I suspect the reading > order implications are a bit hard to understand. > - was it a good idea to use strtok ? > - I went for genericity, but could haved used instead a simple > "set read-ctf-symtab ". If that is better, should > it be a maintenance command or not? > - I initially went for the priority first order in the argument string, so > reflecting the current default would be "dwarf2/ctf+stabs+mdebug", thinking > it would be easier to understand the priority scheme this way, but abandoned > that approach midway. Which is easier to grasp? A point against genericity, it isn't clear if we are supposed to read mdebug+stabs+(dwarf2)/ctf or (mdebug+stabs+dwarf2)/ctf.  The option to change how ctf specifically is read would not create this. About the ordering, I feel like priority order would be less confusing, if you decide to go with the generic option. > > Tested on x86_64-linux. > --- > gdb/elfread.c | 243 ++++++++++++++++++++--- > gdb/testsuite/gdb.ctf/dwarf2-and-ctf-2.c | 24 +++ > gdb/testsuite/gdb.ctf/dwarf2-and-ctf.c | 26 +++ > gdb/testsuite/gdb.ctf/dwarf2-and-ctf.exp | 54 +++++ > 4 files changed, 317 insertions(+), 30 deletions(-) > create mode 100644 gdb/testsuite/gdb.ctf/dwarf2-and-ctf-2.c > create mode 100644 gdb/testsuite/gdb.ctf/dwarf2-and-ctf.c > create mode 100644 gdb/testsuite/gdb.ctf/dwarf2-and-ctf.exp > > diff --git a/gdb/elfread.c b/gdb/elfread.c > index ed11c67dc02..740b0a76513 100644 > --- a/gdb/elfread.c > +++ b/gdb/elfread.c > @@ -51,6 +51,7 @@ > #include "gdbsupport/scoped_fd.h" > #include "debuginfod-support.h" > #include "dwarf2/public.h" > +#include "cli/cli-cmds.h" > > /* The struct elfinfo is available only during ELF symbol table and > psymtab reading. It is destroyed at the completion of psymtab-reading. > @@ -1166,6 +1167,152 @@ elf_read_minimal_symbols (struct objfile *objfile, int symfile_flags, > symtab_create_debug_printf ("done reading minimal symbols"); > } > > +/* Symbol types supported by elfread.c. */ > + > +enum symbol_type > + { > + mdebug, > + stabs, > + dwarf2, > + ctf, > + sep, > + }; > + > +/* Strings corresponding to enum symbol_type. */ > + > +const char *symbol_type_to_string [] = > + { > + "mdebug", > + "stabs", > + "dwarf2", > + "ctf", > + "sep" > + }; > + > +/* Return symbol_type for string S in ST and return true, otherwise return > + false. If ALLOW_SEP is true, we also allow the sep symbol_type. */ > + > +static bool > +string_to_symbol_type (const char *s, enum symbol_type &st, > + bool allow_sep = false) > +{ > + size_t symbol_type_to_string_elems > + = sizeof (symbol_type_to_string) / sizeof (*symbol_type_to_string); > + > + for (unsigned int i = 0; i < symbol_type_to_string_elems; i++) > + { > + if (i == sep && !allow_sep) > + continue; > + > + if (strcmp (symbol_type_to_string[i], s) != 0) > + continue; > + > + st = (symbol_type)i; > + return true; > + } > + > + return false; > +} > + > +/* The default "mdebug+stabs+dwarf2/ctf" means: > + - read mdebug first > + - then read stabs > + - then read dwarf2 > + - then read ctf, but only if dwarf2 is not read. */ > + > +static std::string symbol_read_order_default = "mdebug+stabs+dwarf2/ctf"; > + > +/* Set by the "maint set symbol-read-order " command. */ > + > +static std::string symbol_read_order = symbol_read_order_default; > + > +/* Parsed representation of symbol_read_order. > + For a symbol_read_order of "mdebug+stabs+dwarf2/ctf" we have > + { mdebug, sep, stabs, sep, dwarf, ctf, sep }. */ > + > +std::vector parsed_symbol_read_order; > + > +/* Init parsed_symbol_read_order using symbol_read_order. Return true if > + successful. */ > + > +static bool > +parse_symbol_read_order (void) > +{ > + parsed_symbol_read_order.clear (); > + > + if (symbol_read_order.empty ()) > + return false; > + > + const char *s = symbol_read_order.c_str (); > + > + int prev_delim = -1; > + for (int i = 0; i < symbol_read_order.size (); ++i) > + { > + char c = s[i]; > + if (c != '/' && c != '+') > + continue; > + > + if (i == 0 || i == symbol_read_order.size () - 1) > + /* Cannot begin or end with delimiter. */ > + return false; > + > + if (prev_delim != -1 && prev_delim == i - 1) > + /* Cannot have two subsequent delimiters. */ > + return false; > + > + prev_delim = i; > + } > + > + char s_dup[symbol_read_order.size ()]; > + strcpy (&s_dup[0], s); > + > + for (const char *p = s; *p != '\0'; ++p) > + { > + gdb_assert (*p != '/' && *p != '+'); > + char *token = strtok (p == s ? s_dup : nullptr, "/+"); > + gdb_assert (strlen (token) != 0); > + enum symbol_type st; > + if (!string_to_symbol_type (token, st)) > + { > + parsed_symbol_read_order.clear (); > + return false; > + } > + parsed_symbol_read_order.push_back (st); > + > + p += strlen (token); > + if (*p == '\0') > + { > + parsed_symbol_read_order.push_back (sep); > + break; > + } > + > + if (*p == '+') > + { > + parsed_symbol_read_order.push_back (sep); > + } > + else if (*p == '/') > + continue; > + else > + gdb_assert_not_reached (""); > + } > + > + return true; > +} > + > +static void > +update_symbol_read_order (const char *args, int from_tty, > + struct cmd_list_element *c) > +{ > + if (!parse_symbol_read_order ()) > + { > + warning ("invalid argument: '%s', reverting to default: '%s'", > + symbol_read_order.c_str (), symbol_read_order_default.c_str ()); > + symbol_read_order = symbol_read_order_default; > + bool res = parse_symbol_read_order (); > + gdb_assert (res); > + } > +} > + > /* Dwarf-specific helper for elf_symfile_read. Return true if we managed to > load dwarf debug info. */ > > @@ -1270,6 +1417,12 @@ elf_symfile_read_dwarf2 (struct objfile *objfile, > static void > elf_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags) > { > + if (parsed_symbol_read_order.empty ()) > + { > + bool res = parse_symbol_read_order (); > + gdb_assert (res); > + } > + > bfd *abfd = objfile->obfd.get (); > struct elfinfo ei; > > @@ -1294,38 +1447,59 @@ elf_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags) > we don't do this then the XCOFF info is found first - for code in > an included file XCOFF info is useless. */ > > - if (ei.mdebugsect) > - { > - const struct ecoff_debug_swap *swap; > - > - /* .mdebug section, presumably holding ECOFF debugging > - information. */ > - swap = get_elf_backend_data (abfd)->elf_backend_ecoff_debug_swap; > - if (swap) > - elfmdebug_build_psymtabs (objfile, swap, ei.mdebugsect); > - } > - if (ei.stabsect) > + for (int i = 0; i < parsed_symbol_read_order.size (); ++i) > { > - asection *str_sect; > - > - /* Stab sections have an associated string table that looks like > - a separate section. */ > - str_sect = bfd_get_section_by_name (abfd, ".stabstr"); > - > - /* FIXME should probably warn about a stab section without a stabstr. */ > - if (str_sect) > - elfstab_build_psymtabs (objfile, > - ei.stabsect, > - str_sect->filepos, > - bfd_section_size (str_sect)); > - } > - > - bool has_dwarf2 = elf_symfile_read_dwarf2 (objfile, symfile_flags); > + bool loaded_p = false; > + switch (parsed_symbol_read_order[i]) > + { > + case mdebug: > + if (ei.mdebugsect) > + { > + const struct ecoff_debug_swap *swap; > + > + /* .mdebug section, presumably holding ECOFF debugging > + information. */ > + swap = get_elf_backend_data (abfd)->elf_backend_ecoff_debug_swap; > + if (swap) > + elfmdebug_build_psymtabs (objfile, swap, ei.mdebugsect); > + loaded_p = true; > + } > + break; > + case stabs: > + if (ei.stabsect) > + { > + asection *str_sect; > + > + /* Stab sections have an associated string table that looks like > + a separate section. */ > + str_sect = bfd_get_section_by_name (abfd, ".stabstr"); > + > + /* FIXME should probably warn about a stab section without a stabstr. */ > + if (str_sect) > + elfstab_build_psymtabs (objfile, > + ei.stabsect, > + str_sect->filepos, > + bfd_section_size (str_sect)); > + loaded_p = true; > + } > + break; > + case ctf: > + if (ei.ctfsect) > + { > + elfctf_build_psymtabs (objfile); > + loaded_p = true; > + } > + break; > + case dwarf2: > + loaded_p = elf_symfile_read_dwarf2 (objfile, symfile_flags); > + } > > - /* Read the CTF section only if there is no DWARF info. */ > - if (!has_dwarf2 && ei.ctfsect) > - { > - elfctf_build_psymtabs (objfile); > + if (loaded_p) > + { > + for (i++; i < parsed_symbol_read_order.size (); ++i) > + if (parsed_symbol_read_order[i] == sep) > + break; > + } > } > } > > @@ -1420,4 +1594,13 @@ _initialize_elfread () > add_symtab_fns (bfd_target_elf_flavour, &elf_sym_fns); > > gnu_ifunc_fns_p = &elf_gnu_ifunc_fns; > + > + add_setshow_string_cmd ("symbol-read-order", class_maintenance, > + &symbol_read_order, _("\ > +Set the symbol read order."), _(" \ > +Show the symbol read order."), nullptr, > + update_symbol_read_order, nullptr, > + &maintenance_set_cmdlist, > + &maintenance_show_cmdlist); > + > } > diff --git a/gdb/testsuite/gdb.ctf/dwarf2-and-ctf-2.c b/gdb/testsuite/gdb.ctf/dwarf2-and-ctf-2.c > new file mode 100644 > index 00000000000..834c6d281ae > --- /dev/null > +++ b/gdb/testsuite/gdb.ctf/dwarf2-and-ctf-2.c > @@ -0,0 +1,24 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright 2022 Free Software Foundation, Inc. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see . */ > + > +int var_ctf = 2; > + > +int > +foo (void) > +{ > + return var_ctf; > +} > diff --git a/gdb/testsuite/gdb.ctf/dwarf2-and-ctf.c b/gdb/testsuite/gdb.ctf/dwarf2-and-ctf.c > new file mode 100644 > index 00000000000..d9bfc29dd6d > --- /dev/null > +++ b/gdb/testsuite/gdb.ctf/dwarf2-and-ctf.c > @@ -0,0 +1,26 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright 2022 Free Software Foundation, Inc. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see . */ > + > +int var_dwarf = 1; > + > +extern int foo (); > + > +int > +main (void) > +{ > + return var_dwarf + foo (); > +} > diff --git a/gdb/testsuite/gdb.ctf/dwarf2-and-ctf.exp b/gdb/testsuite/gdb.ctf/dwarf2-and-ctf.exp > new file mode 100644 > index 00000000000..2827babd00d > --- /dev/null > +++ b/gdb/testsuite/gdb.ctf/dwarf2-and-ctf.exp > @@ -0,0 +1,54 @@ > +# Copyright 2022 Free Software Foundation, Inc. > + > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 3 of the License, or > +# (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program. If not, see . > + > +standard_testfile .c -2.c > + > +set objfile [standard_output_file ${testfile}.o] > +set objfile2 [standard_output_file ${testfile}-2.o] > + > +set s1 ${srcdir}/${subdir}/${srcfile} > +set s2 ${srcdir}/${subdir}/${srcfile2} > + > +set opts {} > +lappend opts debug > +if { [gdb_compile $s1 ${objfile} object $opts] != "" } { > + untested "failed to compile" > + return -1 > +} > + > +set opts {} > +lappend opts "additional_flags=-gctf" > +if { [gdb_compile $s2 ${objfile2} object $opts] != "" } { > + untested "failed to compile" > + return -1 > +} > + > +set opts {} > +lappend opts "additional_flags=-Wl,-ctf-variables" > +if { [gdb_compile [list ${objfile} ${objfile2}] $binfile executable $opts] != "" } { > + unsupported "failed to link" > + return -1 > +} > + > +foreach_with_prefix order {dwarf2+ctf ctf+dwarf2} { > + clean_restart > + > + gdb_test "maint set symbol-read-order $order" > + > + gdb_load $binfile > + > + gdb_test "print var_dwarf" " = 1" > + gdb_test "print var_ctf" " = 2" > +}