From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pl1-x633.google.com (mail-pl1-x633.google.com [IPv6:2607:f8b0:4864:20::633]) by sourceware.org (Postfix) with ESMTPS id B88BA386187E for ; Wed, 11 Aug 2021 20:34:28 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org B88BA386187E Received: by mail-pl1-x633.google.com with SMTP id e19so4243555pla.10 for ; Wed, 11 Aug 2021 13:34:28 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=jeR3z8mM7DHSal1EljMgtJMfmJACcwUqRCzhJ6ksI54=; b=CwgUadD20EZwE83UylJLkDPmMYCYR6B3df9kJNGeaI+V5HBWddJmGGGP3YPxCICrfX UKIFMvzmb8S/7HLorlBPpA/aBEwl7HwE5nqRN+o6yHf5+vaa4Hj+KCiTUkxtHKXB5E/S 9SAD8Tx+47a68UluL3r+glI6zceTZrimM/b7ICDXVGN7qNLBudILjn9w38eqYteUaukR XlDmcOSxURlyNOZr+PwUqbyXG3wzJj8fZc11z3D1uGyJ9gKCDiPzUV5k2hf7n8jabNS7 6O5Dk2phbKnMV40WDmLmR/7TdbPf8kRVI0qhR1+pFC+QxR/Fkr1PWmT8PMOZSkYTA240 j39g== X-Gm-Message-State: AOAM532zwNVvgV3W41K6kz4ZusY5nn9VXkGCgKuA8dx0wFO4oQe3m8BG KPfj81Db8MxzeYOdJuRukvzznYQkNMl4BA== X-Google-Smtp-Source: ABdhPJyf7+/dMJqXLjtGgaTSZ2lvoVYZZRrm2sYm7W4VeTMZY61NbLi9NMqdRAQ8Hxg2v9pKjFY9bg== X-Received: by 2002:a17:90a:2bcb:: with SMTP id n11mr408679pje.198.1628714067695; Wed, 11 Aug 2021 13:34:27 -0700 (PDT) Received: from ?IPv6:2804:431:c7cb:9dce:70f4:4f7a:652c:7fb? ([2804:431:c7cb:9dce:70f4:4f7a:652c:7fb]) by smtp.gmail.com with ESMTPSA id h130sm119885pfe.34.2021.08.11.13.34.26 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 11 Aug 2021 13:34:27 -0700 (PDT) Subject: Re: [PATCH v3 5/5] config: Rename HAVE_BUILTIN_MEMSET macro To: Naohiro Tamura , Andreas Schwab , libc-alpha@sourceware.org References: <20210805074733.433430-1-naohirot@fujitsu.com> <20210805075207.433697-1-naohirot@fujitsu.com> From: Adhemerval Zanella Message-ID: <32ca6f4b-5bcf-7578-9d01-1529baa9bebf@linaro.org> Date: Wed, 11 Aug 2021 17:34:25 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <20210805075207.433697-1-naohirot@fujitsu.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.9 required=5.0 tests=BAYES_00, 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_PASS, TXREP 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: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 11 Aug 2021 20:34:39 -0000 On 05/08/2021 04:52, Naohiro Tamura via Libc-alpha wrote: > This patch renames HAVE_BUILTIN_MEMSET macro to > HAVE_NON_LIB_EXPAND_BUILTIN_MEMSET. > > The name "HAVE_BUILTIN_MEMSET" is very confusing. > This macro cannot be removed even though GCC 6.2, that is minimum > requirement to compile glibc, already supports __builtin_memset[1]. > It doesn't indicate whether GCC supports __builtin_memset or not. > > But it indicates whether GCC supports __builtin_memset which doesn't > expand to a memset libcall or not. > > Therefor HAVE_NON_LIB_EXPAND_BUILTIN_MEMSET is more appropriate to > increase code readability. > > [1] https://gcc.gnu.org/onlinedocs/gcc-6.2.0/gcc/Other-Builtins.html Sorry, but I don't see much gain in renaming the internal variable. I agree with the comment improvement. > --- > config.h.in | 4 ++-- > configure | 14 +++++++------- > configure.ac | 10 +++++----- > elf/rtld.c | 9 +++++---- > 4 files changed, 19 insertions(+), 18 deletions(-) > > diff --git a/config.h.in b/config.h.in > index 8b45a3a61d77..4700dc9eba9b 100644 > --- a/config.h.in > +++ b/config.h.in > @@ -40,8 +40,8 @@ > shared between GNU libc and GNU gettext projects. */ > #define HAVE_BUILTIN_EXPECT 1 > > -/* Define if the compiler supports __builtin_memset. */ > -#undef HAVE_BUILTIN_MEMSET > +/* Define if the compiler supports non lib expand __builtin_memset. */ > +#undef HAVE_NON_LIB_EXPAND_BUILTIN_MEMSET > > /* Define if compiler accepts -ftree-loop-distribute-patterns. */ > #undef HAVE_CC_INHIBIT_LOOP_TO_LIBCALL Ok. > diff --git a/configure b/configure > index 9619c10991d0..224c754cf466 100755 > --- a/configure > +++ b/configure > @@ -6263,7 +6263,7 @@ fi > > { $as_echo "$as_me:${as_lineno-$LINENO}: checking for __builtin_memset" >&5 > $as_echo_n "checking for __builtin_memset... " >&6; } > -if ${libc_cv_gcc_builtin_memset+:} false; then : > +if ${libc_cv_gcc_non_lib_expand_builtin_memset+:} false; then : > $as_echo_n "(cached) " >&6 > else > cat > conftest.c <<\EOF > @@ -6279,16 +6279,16 @@ if { ac_try='${CC-cc} -O3 -S conftest.c -o - | grep -F "memset" > /dev/null' > $as_echo "$as_me:${as_lineno-$LINENO}: \$? = $ac_status" >&5 > test $ac_status = 0; }; }; > then > - libc_cv_gcc_builtin_memset=no > + libc_cv_gcc_non_lib_expand_builtin_memset=no > else > - libc_cv_gcc_builtin_memset=yes > + libc_cv_gcc_non_lib_expand_builtin_memset=yes > fi > rm -f conftest* > fi > -{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $libc_cv_gcc_builtin_memset" >&5 > -$as_echo "$libc_cv_gcc_builtin_memset" >&6; } > -if test "$libc_cv_gcc_builtin_memset" = yes ; then > - $as_echo "#define HAVE_BUILTIN_MEMSET 1" >>confdefs.h > +{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $libc_cv_gcc_non_lib_expand_builtin_memset" >&5 > +$as_echo "$libc_cv_gcc_non_lib_expand_builtin_memset" >&6; } > +if test "$libc_cv_gcc_non_lib_expand_builtin_memset" = yes ; then > + $as_echo "#define HAVE_NON_LIB_EXPAND_BUILTIN_MEMSET 1" >>confdefs.h > > fi > > diff --git a/configure.ac b/configure.ac > index 34ecbba54054..451cea7683e6 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -1508,7 +1508,7 @@ if test $libc_cv_have_section_quotes = yes; then > AC_DEFINE(HAVE_SECTION_QUOTES) > fi > > -AC_CACHE_CHECK(for __builtin_memset, libc_cv_gcc_builtin_memset, [dnl > +AC_CACHE_CHECK(for __builtin_memset, libc_cv_gcc_non_lib_expand_builtin_memset, [dnl > cat > conftest.c <<\EOF > void zero (void *x) > { > @@ -1518,13 +1518,13 @@ EOF > dnl > if AC_TRY_COMMAND([${CC-cc} -O3 -S conftest.c -o - | grep -F "memset" > /dev/null]); > then > - libc_cv_gcc_builtin_memset=no > + libc_cv_gcc_non_lib_expand_builtin_memset=no > else > - libc_cv_gcc_builtin_memset=yes > + libc_cv_gcc_non_lib_expand_builtin_memset=yes > fi > rm -f conftest* ]) > -if test "$libc_cv_gcc_builtin_memset" = yes ; then > - AC_DEFINE(HAVE_BUILTIN_MEMSET) > +if test "$libc_cv_gcc_non_lib_expand_builtin_memset" = yes ; then > + AC_DEFINE(HAVE_NON_LIB_EXPAND_BUILTIN_MEMSET) > fi > > AC_CACHE_CHECK(for redirection of built-in functions, libc_cv_gcc_builtin_redirection, [dnl I see no point in rename it, a better comment as you are doing is suffice. > diff --git a/elf/rtld.c b/elf/rtld.c > index d733359eaf80..a18494fcd38e 100644 > --- a/elf/rtld.c > +++ b/elf/rtld.c > @@ -527,11 +527,12 @@ _dl_start (void *arg) > /* Partly clean the `bootstrap_map' structure up. Don't use > `memset' since it might not be built in or inlined and we cannot > make function calls at this point. Use '__builtin_memset' if we > - know it is available. We do not have to clear the memory if we > - do not have to use the temporary bootstrap_map. Global variables > - are initialized to zero by default. */ > + know it is available and does not expand to a memset libcall. > + We do not have to clear the memory if we do not have to use the > + temporary bootstrap_map. Global variables are initialized to > + zero by default. */ > #ifndef DONT_USE_BOOTSTRAP_MAP > -# ifdef HAVE_BUILTIN_MEMSET > +# ifdef HAVE_NON_LIB_EXPAND_BUILTIN_MEMSET > __builtin_memset (bootstrap_map.l_info, '\0', sizeof (bootstrap_map.l_info)); > # else > for (size_t cnt = 0; > Ok.