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.129.124]) by sourceware.org (Postfix) with ESMTPS id 1385F3858C78 for ; Fri, 17 Feb 2023 09:18:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 1385F3858C78 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1676625518; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=IdMDlzam4crA3EfpK+NSdoIjMU4QtNZJ86GJjYaMxFc=; b=gy7GQOnYTG/WVg9A4+ShI6dC3nBn7BrIz44aNB28pIRPX4UI5L7i1XlYjas9eBBqHOBYPu I1Ta72jauOmYdy8xx9DzzfAwOlKQzEgkB5PujUOc6w+a7jhBvNFffGgHqzJ1nNjhaan0L5 WCMa5r4EiOytO502sYAgyzVJOlKYyJg= Received: from mail-wr1-f71.google.com (mail-wr1-f71.google.com [209.85.221.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-391-Idp2PV-wO6eLwSAIwXMGtA-1; Fri, 17 Feb 2023 04:18:37 -0500 X-MC-Unique: Idp2PV-wO6eLwSAIwXMGtA-1 Received: by mail-wr1-f71.google.com with SMTP id 2-20020a056000156200b002c54ce46094so42250wrz.17 for ; Fri, 17 Feb 2023 01:18:36 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:references :in-reply-to:subject:to:from:x-gm-message-state:from:to:cc:subject :date:message-id:reply-to; bh=FlgBb2ZDxqkAI8LkUrFSeUuTTaDes57bYL98GIv5ICw=; b=3UAEE5AyAA0iV+stVst8pa1O50uReyGsKxhth+TXMdCdTMqR8BnOyiRfpW12nXK9E/ Hef3D5+yZ+iMk4J0XwH8b22VllpHbXGybPCoorqRAclVs3xNfMxkYoA6FpUAt2YO6coM BeUaB6rgGoeTJ7o5cOgIekmQFNtRyWxpf//rGD7orcfWdbNevxMcHFAbF4KsBoz+J3Xh +wNkZufodEMW3a776NSRT1Kw5PvHjyUU9mNqPDbtcjw9rlucwBZFuomJV9Hw2Kkpa38Q tw3xxJOUx+wjofTvzTzmitt0B4RZzyJXnG29IHVam720YG1pUP0Duc6Ydrg4wNB+XFbp zVCw== X-Gm-Message-State: AO0yUKXsT1DiobxLiZ76cFGNaI7CzyWO+7l4IAPoZGUIAPXivFedheq3 IoiSK/g/enhNhZ1RSs5W0/s2kJ3+fn3wDXwrMno99GaZoQmNtyDx7lcjzU2Hv99MIOsW6ZGIs7p aBJsgOeE5fqsHNMN8Ce6Bn8g3IWQ= X-Received: by 2002:a5d:660f:0:b0:2c5:8505:be46 with SMTP id n15-20020a5d660f000000b002c58505be46mr3730358wru.24.1676625515680; Fri, 17 Feb 2023 01:18:35 -0800 (PST) X-Google-Smtp-Source: AK7set9iNqbTXq59wI2DpZmN2DWSb+DcYalZQnaMjuiuabd5tVQUaNC3MldAHRvI87BPkzNPBSO3tg== X-Received: by 2002:a5d:660f:0:b0:2c5:8505:be46 with SMTP id n15-20020a5d660f000000b002c58505be46mr3730339wru.24.1676625515291; Fri, 17 Feb 2023 01:18:35 -0800 (PST) Received: from localhost (95.72.115.87.dyn.plus.net. [87.115.72.95]) by smtp.gmail.com with ESMTPSA id m18-20020adfe952000000b002c56a7872f4sm3664114wrn.82.2023.02.17.01.18.34 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 17 Feb 2023 01:18:34 -0800 (PST) From: Andrew Burgess To: Alexandra =?utf-8?B?SMOhamtvdsOh?= , gdb-patches@sourceware.org Subject: Re: [PATCH] Use styled_string when defering warnings when loading separate debug files In-Reply-To: <20230216195604.2685177-1-ahajkova@redhat.com> References: <20230216195604.2685177-1-ahajkova@redhat.com> Date: Fri, 17 Feb 2023 09:18:33 +0000 Message-ID: <87r0uolkeu.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-11.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,RCVD_IN_DNSWL_NONE,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 List-Id: Alexandra H=C3=A1jkov=C3=A1 via Gdb-patches wr= ites: > This improves commit 6647f05df023b63bbe056e9167e9e234172fa2ca, the > filenames used in the warnings are styled properly now. > --- > gdb/build-id.c | 19 ++++++++++++++++--- > gdb/build-id.h | 5 ++++- > gdb/coffread.c | 6 +++--- > gdb/elfread.c | 6 +++--- > gdb/symfile.c | 21 ++++++++++++++++----- > gdb/symfile.h | 5 ++++- > 6 files changed, 46 insertions(+), 16 deletions(-) > > diff --git a/gdb/build-id.c b/gdb/build-id.c > index 00250c20ae9..74605bb702c 100644 > --- a/gdb/build-id.c > +++ b/gdb/build-id.c > @@ -18,6 +18,7 @@ > along with this program. If not, see .= */ > =20 > #include "defs.h" > +#include "cli/cli-style.h" > #include "bfd.h" > #include "gdb_bfd.h" > #include "build-id.h" > @@ -205,11 +206,21 @@ build_id_to_exec_bfd (size_t build_id_len, const bf= d_byte *build_id) > return build_id_to_bfd_suffix (build_id_len, build_id, ""); > } > =20 > +static void > +add_warning (warnings_vec *vec, std::string msg) This function should have a comment just above it. > +{ > + vec->emplace_back ([msg] () { > +=09=09 gdb_printf (gdb_stdlog, "%ps", > +=09=09=09=09 styled_string (file_name_style. style (), > +=09=09=09=09=09=09msg.c_str ())); > +=09=09 }); > +} > + This is going to style the whole of MSG with the file_name_style, this isn't what you want.... > /* See build-id.h. */ > =20 > std::string > find_separate_debug_file_by_buildid (struct objfile *objfile, > -=09=09=09=09 std::vector *warnings_vector) > +=09=09=09=09 warnings_vec *vec) > { > const struct bfd_build_id *build_id; > =20 > @@ -232,8 +243,10 @@ find_separate_debug_file_by_buildid (struct objfile = *objfile, > =09 =3D string_printf (_("\"%s\": separate debug info file has no " > =09=09=09 "debug info"), bfd_get_filename (abfd.get ())); > =09 if (separate_debug_file_debug) > -=09 gdb_printf (gdb_stdlog, "%s", msg.c_str ()); > -=09 warnings_vector->emplace_back (std::move (msg)); > +=09 gdb_printf (gdb_stdlog, "%ps", > +=09=09=09styled_string (file_name_style. style (), > +=09=09=09=09 msg.c_str ())); ... again here you're styling the whole string, which is wrong. Also, we shouldn't be styling strings (or we don't traditionally) style strings in debug output. > +=09 add_warning(vec, msg); You need something like: warnings->emplace_back ([msg] () { warning (_("\"%ps\": separate debug info file has no debug info"= ), styled_string (file_name_style.style (), bfd_get_filename (abfd.get ()))); Which will just style the filename part of the output. > =09} > else if (abfd !=3D NULL) > =09return std::string (bfd_get_filename (abfd.get ())); > diff --git a/gdb/build-id.h b/gdb/build-id.h > index 191720ddf28..8e72e6ecff6 100644 > --- a/gdb/build-id.h > +++ b/gdb/build-id.h > @@ -47,6 +47,9 @@ extern gdb_bfd_ref_ptr build_id_to_debug_bfd (size_t bu= ild_id_len, > extern gdb_bfd_ref_ptr build_id_to_exec_bfd (size_t build_id_len, > =09=09=09=09=09 const bfd_byte *build_id); > =20 > +using warning_cb =3D std::function; > +using warnings_vec =3D std::vector; > + > /* Find the separate debug file for OBJFILE, by using the build-id > associated with OBJFILE's BFD. If successful, returns the file name = for the > separate debug file, otherwise, return an empty string. > @@ -58,7 +61,7 @@ extern gdb_bfd_ref_ptr build_id_to_exec_bfd (size_t bui= ld_id_len, > approach, then any warnings will be printed. */ > =20 > extern std::string find_separate_debug_file_by_buildid > - (struct objfile *objfile, std::vector *warnings_vector); > + (struct objfile *objfile, warnings_vec *vec); > =20 > /* Return an hex-string representation of BUILD_ID. */ > =20 > diff --git a/gdb/coffread.c b/gdb/coffread.c > index 65d7828e933..c534eb504cf 100644 > --- a/gdb/coffread.c > +++ b/gdb/coffread.c > @@ -734,7 +734,7 @@ coff_symfile_read (struct objfile *objfile, symfile_a= dd_flags symfile_flags) > /* Try to add separate debug file if no symbols table found. */ > if (!objfile->has_partial_symbols ()) > { > - std::vector warnings_vector; > + warnings_vec warnings_vector; > std::string debugfile > =09=3D find_separate_debug_file_by_buildid (objfile, &warnings_vector); > =20 > @@ -752,8 +752,8 @@ coff_symfile_read (struct objfile *objfile, symfile_a= dd_flags symfile_flags) > /* If all the methods to collect the debuginfo failed, print any > =09 warnings that were collected. */ > if (debugfile.empty () && !warnings_vector.empty ()) > -=09for (const std::string &w : warnings_vector) > -=09 warning ("%s", w.c_str ()); > +=09for (const auto &cb : warnings_vector) > +=09 cb (); > } > } > =20 > diff --git a/gdb/elfread.c b/gdb/elfread.c > index ca684aab57e..d1c231c2f8c 100644 > --- a/gdb/elfread.c > +++ b/gdb/elfread.c > @@ -1213,7 +1213,7 @@ elf_symfile_read_dwarf2 (struct objfile *objfile, > =09 && objfile->separate_debug_objfile =3D=3D NULL > =09 && objfile->separate_debug_objfile_backlink =3D=3D NULL) > { > - std::vector warnings_vector; > + warnings_vec warnings_vector; > =20 > std::string debugfile > =09=3D find_separate_debug_file_by_buildid (objfile, &warnings_vector); > @@ -1265,8 +1265,8 @@ elf_symfile_read_dwarf2 (struct objfile *objfile, > /* If all the methods to collect the debuginfo failed, print > =09 the warnings, if there're any. */ > if (debugfile.empty () && !has_dwarf2 && !warnings_vector.empty ()= ) > -=09for (const std::string &w : warnings_vector) > -=09 warning ("%s", w.c_str ()); > +=09for (const auto &cb : warnings_vector) > +=09 cb (); > } > =20 > return has_dwarf2; > diff --git a/gdb/symfile.c b/gdb/symfile.c > index 373f5592107..1fcb5fd5ae4 100644 > --- a/gdb/symfile.c > +++ b/gdb/symfile.c > @@ -1241,10 +1241,20 @@ symbol_file_clear (int from_tty) > =20 > bool separate_debug_file_debug =3D false; > =20 > +static void > +add_warning (warnings_vec *vec, std::string msg) > +{ > + vec->emplace_back ([msg] () { > +=09=09 gdb_printf (gdb_stdlog, "warning: %ps", > +=09=09=09=09 styled_string (file_name_style. style (), > +=09=09=09=09=09=09msg.c_str ())); > +=09=09 }); > +} > + > static int > separate_debug_file_exists (const std::string &name, unsigned long crc, > =09=09=09 struct objfile *parent_objfile, > -=09=09=09 std::vector *warnings_vector) > +=09=09=09 warnings_vec *warnings_vector) > { > unsigned long file_crc; > int file_crc_p; > @@ -1342,8 +1352,9 @@ separate_debug_file_exists (const std::string &name= , unsigned long crc, > =09=09=09 " does not match \"%s\" (CRC mismatch).\n"), > =09=09=09 name.c_str (), objfile_name (parent_objfile)); > =09 if (separate_debug_file_debug) > -=09 gdb_printf (gdb_stdlog, "%s", msg.c_str ()); > -=09 warnings_vector->emplace_back (std::move (msg)); > +=09 gdb_printf (gdb_stdlog, "%ps", styled_string (file_name_style. st= yle (), > +=09=09=09=09=09=09=09 msg.c_str ())); > +=09 add_warning(warnings_vector, msg); > =09} > =20 > return 0; > @@ -1386,7 +1397,7 @@ find_separate_debug_file (const char *dir, > =09=09=09 const char *canon_dir, > =09=09=09 const char *debuglink, > =09=09=09 unsigned long crc32, struct objfile *objfile, > -=09=09=09 std::vector *warnings_vector) > +=09=09=09 warnings_vec *warnings_vector) > { > if (separate_debug_file_debug) > gdb_printf (gdb_stdlog, > @@ -1534,7 +1545,7 @@ terminate_after_last_dir_separator (char *path) > =20 > std::string > find_separate_debug_file_by_debuglink > - (struct objfile *objfile, std::vector *warnings_vector) > + (struct objfile *objfile, warnings_vec *warnings_vector) > { > unsigned long crc32; > =20 > diff --git a/gdb/symfile.h b/gdb/symfile.h > index b433e2be31a..0cb12176152 100644 > --- a/gdb/symfile.h > +++ b/gdb/symfile.h > @@ -241,6 +241,9 @@ extern struct objfile *symbol_file_add_from_bfd (cons= t gdb_bfd_ref_ptr &, > extern void symbol_file_add_separate (const gdb_bfd_ref_ptr &, const cha= r *, > =09=09=09=09 symfile_add_flags, struct objfile *); > =20 > +using warning_cb =3D std::function; > +using warnings_vec =3D std::vector; You should avoid creating duplicate type definitions like this, as this invites problems where one is updated and not the other. Pick one location and define the types there, and just make sure the header file is included where needed. Of the two locations you've used, I'd go with symfile.h, as that seems the more general of the two. Additionally ... comments. Maybe a single comment above both would be fine? Thanks, Andrew > + > /* Find separate debuginfo for OBJFILE (using .gnu_debuglink section). > Returns pathname, or an empty string. > =20 > @@ -248,7 +251,7 @@ extern void symbol_file_add_separate (const gdb_bfd_r= ef_ptr &, const char *, > WARNINGS_VECTOR, one std::string per warning. */ > =20 > extern std::string find_separate_debug_file_by_debuglink > - (struct objfile *objfile, std::vector *warnings_vector); > + (struct objfile *objfile, warnings_vec *warnings_vector); > =20 > /* Build (allocate and populate) a section_addr_info struct from an > existing section table. */ > --=20 > 2.39.1