From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x42e.google.com (mail-wr1-x42e.google.com [IPv6:2a00:1450:4864:20::42e]) by sourceware.org (Postfix) with ESMTPS id B2CD43857436 for ; Wed, 7 Jul 2021 08:43:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org B2CD43857436 Received: by mail-wr1-x42e.google.com with SMTP id u8so2078203wrq.8 for ; Wed, 07 Jul 2021 01:43:25 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=scZVYFRnQxFCLoDmIaTgtjIqhzSimKGFGR9BRWrxMz0=; b=AoX0yr22h3/YHSGYg9GIdmS/paofZ0dLJEm12N2/gSa/M2UE1LJLEPsOmZnuIa/E6B Fkl++JrF3X+t7WUEcRpsYY8oUkukCgdTl2TflftGUcELfK2sUemr6uImY1u3nNReJdsi VGZ/N1rqW2mXlVlGtexw22vK9ZFT0+DmKg0A+Y9dTBlXqanTcC/9r2DdkW7IU871dpKZ s2NEMeLq1RLfcrHLd7HjkHgbb7BhjY+mWfYog9ANkQn0A91dFnSndDvME5kRrcG5lRQA kE5QSgDuhNdv8KjcHl+kiGuQ+Pv79d2M3RhX3+RJa94sFv8ENEOlXVbecXuXetDy/tn2 AVUA== X-Gm-Message-State: AOAM531H4jimOLX9XrxHWU8IuCdNo+gJIc1izh9CJhdjNH+kHXCJlhwx FkZedBKpAwxAy+86ozRDAEXuHA== X-Google-Smtp-Source: ABdhPJzbTFNxWJMc8Jg8COFJ+HYKQVrvWRxd/sk3cMWIDZBJT438ru5ZyFRRqi39n0V+gRW6fhTZHA== X-Received: by 2002:a5d:4cc4:: with SMTP id c4mr23770514wrt.128.1625647404529; Wed, 07 Jul 2021 01:43:24 -0700 (PDT) Received: from google.com ([2a00:79e0:d:210:67d9:6937:aa95:70a4]) by smtp.gmail.com with ESMTPSA id t9sm19432599wrq.92.2021.07.07.01.43.23 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 07 Jul 2021 01:43:24 -0700 (PDT) Date: Wed, 7 Jul 2021 09:43:23 +0100 From: Matthias Maennich To: Giuliano Procida Cc: libabigail@sourceware.org, dodji@seketeli.org, kernel-team@android.com Subject: Re: [PATCH] elf_symbol: remove "is Linux string constant" property Message-ID: References: <20210706163602.307939-1-gprocida@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <20210706163602.307939-1-gprocida@google.com> X-Spam-Status: No, score=-30.1 required=5.0 tests=BAYES_00, DKIMWL_WL_MED, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, ENV_AND_HDR_SPF_MATCH, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, USER_IN_DEF_DKIM_WL, USER_IN_DEF_SPF_WL autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: libabigail@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Mailing list of the Libabigail project List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 07 Jul 2021 08:43:27 -0000 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 Thanks for clearing up this leftover TODO! Reviewed-by: Matthias Maennich 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 >