From: Matthias Maennich <maennich@google.com>
To: Giuliano Procida <gprocida@google.com>
Cc: libabigail@sourceware.org, dodji@seketeli.org, kernel-team@android.com
Subject: Re: [PATCH] elf_symbol: remove "is Linux string constant" property
Date: Wed, 7 Jul 2021 09:43:23 +0100 [thread overview]
Message-ID: <YOVpKxsX0tUg4iCA@google.com> (raw)
In-Reply-To: <20210706163602.307939-1-gprocida@google.com>
On Tue, Jul 06, 2021 at 05:36:02PM +0100, Giuliano Procida wrote:
>This boolean property was obsoleted by the new symtab reader
>implementation. It has no users.
>
>Following this change, the find_ksymtab_strings_section function joins
>find_ksymtab_section and find_ksymtab_gpl_section in having no users.
>
> * include/abg-ir.h (elf_symbol::elf_symbol): Drop
> is_linux_string_cst argument.
> (elf_symbol::create): Likewise.
> (elf_symbol::get_is_linux_string_cst): Drop method.
> * src/abg-dwarf-reader.cc (lookup_symbol_from_sysv_hash_tab):
> Remove code that gets the index of the __ksymtab_strings
> section. Drop corresponding elf_symbol::create argument.
> (lookup_symbol_from_gnu_hash_tab): Likewise.
> (lookup_symbol_from_symtab): Likewise.
> (create_default_fn_sym): Drop false is_linux_string_cst
> argument to elf_symbol::create.
> * src/abg-ir.cc (elf_symbol::priv): Drop is_linux_string_cst_
> member variable.
> (elf_symbol::priv default ctor): Drop initialisation of
> is_linux_string_cst_.
> (elf_symbol::priv normal ctor): Drop is_linux_string_cst
> argument and corresponding is_linux_string_cst_
> initialisation.
> (elf_symbol::elf_symbol ctor): Drop is_linux_string_cst
> argument and corresponding forwarding to priv ctor.
> (elf_symbol::create): Drop is_linux_string_cst argument and
> corresponding forwarding to ctor.
> (elf_symbol::get_is_linux_string_cst): Drop method.
> * src/abg-reader.cc (build_elf_symbol): Drop false
> is_linux_string_cst argument to elf_symbol::create.
> * src/abg-symtab-reader.cc (symtab::load): Likewise.
>
>Signed-off-by: Giuliano Procida <gprocida@google.com>
Thanks for clearing up this leftover TODO!
Reviewed-by: Matthias Maennich <maennich@google.com>
Cheers,
Matthias
>---
> include/abg-ir.h | 5 -----
> src/abg-dwarf-reader.cc | 24 ++++--------------------
> src/abg-ir.cc | 23 -----------------------
> src/abg-reader.cc | 3 +--
> src/abg-symtab-reader.cc | 5 +----
> 5 files changed, 6 insertions(+), 54 deletions(-)
>
>diff --git a/include/abg-ir.h b/include/abg-ir.h
>index da43727f..7ab3b2b7 100644
>--- a/include/abg-ir.h
>+++ b/include/abg-ir.h
>@@ -922,7 +922,6 @@ private:
> bool c,
> const version& ve,
> visibility vi,
>- bool is_linux_string_cst = false,
> bool is_in_ksymtab = false,
> uint64_t crc = 0,
> bool is_suppressed = false);
>@@ -948,7 +947,6 @@ public:
> bool c,
> const version& ve,
> visibility vi,
>- bool is_linux_string_cst = false,
> bool is_in_ksymtab = false,
> uint64_t crc = 0,
> bool is_suppressed = false);
>@@ -965,9 +963,6 @@ public:
> void
> set_index(size_t);
>
>- bool
>- get_is_linux_string_cst() const;
>-
> const string&
> get_name() const;
>
>diff --git a/src/abg-dwarf-reader.cc b/src/abg-dwarf-reader.cc
>index c8cd5170..bcb3883a 100644
>--- a/src/abg-dwarf-reader.cc
>+++ b/src/abg-dwarf-reader.cc
>@@ -824,10 +824,6 @@ lookup_symbol_from_sysv_hash_tab(const environment* env,
> elf_symbol::binding sym_binding;
> elf_symbol::visibility sym_visibility;
> bool found = false;
>- Elf_Scn *strings_section = find_ksymtab_strings_section(elf_handle);
>- size_t strings_ndx = strings_section
>- ? elf_ndxscn(strings_section)
>- : 0;
>
> do
> {
>@@ -856,8 +852,7 @@ lookup_symbol_from_sysv_hash_tab(const environment* env,
> sym_binding,
> symbol.st_shndx != SHN_UNDEF,
> symbol.st_shndx == SHN_COMMON,
>- ver, sym_visibility,
>- symbol.st_shndx == strings_ndx);
>+ ver, sym_visibility);
> syms_found.push_back(symbol_found);
> found = true;
> }
>@@ -1103,10 +1098,6 @@ lookup_symbol_from_gnu_hash_tab(const environment* env,
> elf_symbol::type sym_type;
> elf_symbol::binding sym_binding;
> elf_symbol::visibility sym_visibility;
>- Elf_Scn *strings_section = find_ksymtab_strings_section(elf_handle);
>- size_t strings_ndx = strings_section
>- ? elf_ndxscn(strings_section)
>- : 0;
>
> // Let's walk the hash table and record the versions of all the
> // symbols which name equal sym_name.
>@@ -1151,8 +1142,7 @@ lookup_symbol_from_gnu_hash_tab(const environment* env,
> sym_type, sym_binding,
> symbol.st_shndx != SHN_UNDEF,
> symbol.st_shndx == SHN_COMMON,
>- ver, sym_visibility,
>- symbol.st_shndx == strings_ndx);
>+ ver, sym_visibility);
> syms_found.push_back(symbol_found);
> found = true;
> }
>@@ -1275,10 +1265,6 @@ lookup_symbol_from_symtab(const environment* env,
> char* name_str = 0;
> elf_symbol::version ver;
> bool found = false;
>- Elf_Scn *strings_section = find_ksymtab_strings_section(elf_handle);
>- size_t strings_ndx = strings_section
>- ? elf_ndxscn(strings_section)
>- : 0;
>
> for (size_t i = 0; i < symcount; ++i)
> {
>@@ -1307,8 +1293,7 @@ lookup_symbol_from_symtab(const environment* env,
> elf_symbol::create(env, i, sym->st_size,
> name_str, sym_type,
> sym_binding, sym_is_defined,
>- sym_is_common, ver, sym_visibility,
>- sym->st_shndx == strings_ndx);
>+ sym_is_common, ver, sym_visibility);
> syms_found.push_back(symbol_found);
> found = true;
> }
>@@ -14118,8 +14103,7 @@ create_default_fn_sym(const string& sym_name, const environment *env)
> /*symbol is defined=*/ true,
> /*symbol is common=*/ false,
> /*symbol version=*/ ver,
>- /*symbol visibility=*/elf_symbol::DEFAULT_VISIBILITY,
>- /*symbol is linux string cst=*/false);
>+ /*symbol visibility=*/elf_symbol::DEFAULT_VISIBILITY);
> return result;
> }
>
>diff --git a/src/abg-ir.cc b/src/abg-ir.cc
>index f8664566..d3134744 100644
>--- a/src/abg-ir.cc
>+++ b/src/abg-ir.cc
>@@ -1395,7 +1395,6 @@ struct elf_symbol::priv
> // STT_COMMON. If no such symbol is found, it looks for the
> // STT_COMMON definition of that name that has the largest size.
> bool is_common_;
>- bool is_linux_string_cst_;
> bool is_in_ksymtab_;
> uint64_t crc_;
> bool is_suppressed_;
>@@ -1413,7 +1412,6 @@ struct elf_symbol::priv
> visibility_(elf_symbol::DEFAULT_VISIBILITY),
> is_defined_(false),
> is_common_(false),
>- is_linux_string_cst_(false),
> is_in_ksymtab_(false),
> crc_(0),
> is_suppressed_(false)
>@@ -1429,7 +1427,6 @@ struct elf_symbol::priv
> bool c,
> const elf_symbol::version& ve,
> elf_symbol::visibility vi,
>- bool is_linux_string_cst,
> bool is_in_ksymtab,
> uint64_t crc,
> bool is_suppressed)
>@@ -1443,7 +1440,6 @@ struct elf_symbol::priv
> visibility_(vi),
> is_defined_(d),
> is_common_(c),
>- is_linux_string_cst_(is_linux_string_cst),
> is_in_ksymtab_(is_in_ksymtab),
> crc_(crc),
> is_suppressed_(is_suppressed)
>@@ -1490,9 +1486,6 @@ elf_symbol::elf_symbol()
> ///
> /// @param vi the visibility of the symbol.
> ///
>-/// @param is_linux_string_cst true if the symbol is a Linux Kernel
>-/// string constant defined in the __ksymtab_strings section.
>-///
> /// @param crc the CRC (modversions) value of Linux Kernel symbols
> elf_symbol::elf_symbol(const environment* e,
> size_t i,
>@@ -1504,7 +1497,6 @@ elf_symbol::elf_symbol(const environment* e,
> bool c,
> const version& ve,
> visibility vi,
>- bool is_linux_string_cst,
> bool is_in_ksymtab,
> uint64_t crc,
> bool is_suppressed)
>@@ -1518,7 +1510,6 @@ elf_symbol::elf_symbol(const environment* e,
> c,
> ve,
> vi,
>- is_linux_string_cst,
> is_in_ksymtab,
> crc,
> is_suppressed))
>@@ -1562,9 +1553,6 @@ elf_symbol::create()
> ///
> /// @param vi the visibility of the symbol.
> ///
>-/// @param is_linux_string_cst if true, it means the symbol represents
>-/// a string constant from a linux kernel binary.
>-///
> /// @param crc the CRC (modversions) value of Linux Kernel symbols
> ///
> /// @return a (smart) pointer to a newly created instance of @ref
>@@ -1580,13 +1568,11 @@ elf_symbol::create(const environment* e,
> bool c,
> const version& ve,
> visibility vi,
>- bool is_linux_string_cst,
> bool is_in_ksymtab,
> uint64_t crc,
> bool is_suppressed)
> {
> elf_symbol_sptr sym(new elf_symbol(e, i, s, n, t, b, d, c, ve, vi,
>- is_linux_string_cst,
> is_in_ksymtab, crc, is_suppressed));
> sym->priv_->main_symbol_ = sym;
> return sym;
>@@ -1653,15 +1639,6 @@ void
> elf_symbol::set_index(size_t s)
> {priv_->index_ = s;}
>
>-/// Test if the ELF symbol is for a string constant of a Linux binary
>-/// defined in the __ksymtab_strings symbol table.
>-///
>-/// @return true iff ELF symbol is for a string constant of a Linux
>-/// binary defined in the __ksymtab_strings symbol table.
>-bool
>-elf_symbol::get_is_linux_string_cst() const
>-{return priv_->is_linux_string_cst_;}
>-
> /// Getter for the name of the @ref elf_symbol.
> ///
> /// @return a reference to the name of the @ref symbol.
>diff --git a/src/abg-reader.cc b/src/abg-reader.cc
>index 0b8149f6..6af4d404 100644
>--- a/src/abg-reader.cc
>+++ b/src/abg-reader.cc
>@@ -2985,8 +2985,7 @@ build_elf_symbol(read_context& ctxt, const xmlNodePtr node,
> elf_symbol_sptr e = elf_symbol::create(env, /*index=*/0,
> size, name, type, binding,
> is_defined, is_common,
>- version, visibility,
>- /*is_linux_string_cst=*/false);
>+ version, visibility);
>
> e->set_is_suppressed(is_suppressed);
>
>diff --git a/src/abg-symtab-reader.cc b/src/abg-symtab-reader.cc
>index edc9d825..4cc40d28 100644
>--- a/src/abg-symtab-reader.cc
>+++ b/src/abg-symtab-reader.cc
>@@ -315,10 +315,7 @@ symtab::load_(Elf* elf_handle,
> elf_helpers::stb_to_elf_symbol_binding(GELF_ST_BIND(sym->st_info)),
> sym_is_defined, sym_is_common, ver,
> elf_helpers::stv_to_elf_symbol_visibility
>- (GELF_ST_VISIBILITY(sym->st_other)),
>- /*is_linux_strings_cstr=*/false); // TODO: remove
>- // is_linux_strings_cstr
>- // as it is obsolete
>+ (GELF_ST_VISIBILITY(sym->st_other)));
>
> // We do not take suppressed symbols into our symbol vector to avoid
> // accidental leakage. But we ensure supressed symbols are otherwise set
>--
>2.32.0.93.g670b81a890-goog
>
next prev parent reply other threads:[~2021-07-07 8:43 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-06 16:36 Giuliano Procida
2021-07-07 8:43 ` Matthias Maennich [this message]
2021-07-16 15:18 ` [PATCH, applied] " Dodji Seketeli
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YOVpKxsX0tUg4iCA@google.com \
--to=maennich@google.com \
--cc=dodji@seketeli.org \
--cc=gprocida@google.com \
--cc=kernel-team@android.com \
--cc=libabigail@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).