From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-17.5 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7781FC433DB for ; Mon, 22 Mar 2021 08:28:09 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id A42E061967 for ; Mon, 22 Mar 2021 08:28:08 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org A42E061967 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Received: from localhost ([::1]:40274 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1lOFux-0006k2-JS for qemu-devel@archiver.kernel.org; Mon, 22 Mar 2021 04:28:07 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:59596) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lOFu4-000637-JX for qemu-devel@nongnu.org; Mon, 22 Mar 2021 04:27:12 -0400 Received: from us-smtp-delivery-124.mimecast.com ([63.128.21.124]:60398) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lOFtv-00035M-Oe for qemu-devel@nongnu.org; Mon, 22 Mar 2021 04:27:11 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1616401622; h=from:from:reply-to:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=2EJoCR8oTRLLlf4GbQtzyxOWC0heCiR/4lIYUShFTQM=; b=O1Fx3WSXfUGvdMkj6c4dlDeJtUno1yjQYChRPoL3xa0JIHVKzgo/BZHpoy1ne8zLplE9hv xTVpxIBzHGyNFMOkIfK6e25X/pBOg70A0QhkvUruZV92jVJ2Agyi200Xwyo9KuqfKNIUD1 h7MD87xLqnvSVy5hTF15gUP6rDXvgP4= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-322-D2GfVo96OAaR_ANe2cwoig-1; Mon, 22 Mar 2021 04:26:59 -0400 X-MC-Unique: D2GfVo96OAaR_ANe2cwoig-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 1EEA2107ACCD; Mon, 22 Mar 2021 08:26:58 +0000 (UTC) Received: from [10.64.54.40] (vpn2-54-40.bne.redhat.com [10.64.54.40]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 3C80060BE5; Mon, 22 Mar 2021 08:26:55 +0000 (UTC) Subject: Re: [PATCH] exec: Build page-varry-common.c with -fno-lto To: Richard Henderson , qemu-devel@nongnu.org References: <20210321211534.2101231-1-richard.henderson@linaro.org> From: Gavin Shan Message-ID: <32ae3eb4-4a14-0610-e31b-0d9bf12a0da8@redhat.com> Date: Mon, 22 Mar 2021 22:26:28 +1100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.0 MIME-Version: 1.0 In-Reply-To: <20210321211534.2101231-1-richard.henderson@linaro.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=gshan@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Received-SPF: pass client-ip=63.128.21.124; envelope-from=gshan@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -27 X-Spam_score: -2.8 X-Spam_bar: -- X-Spam_report: (-2.8 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.001, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, NICE_REPLY_A=-0.001, RCVD_IN_DNSWL_LOW=-0.7, RCVD_IN_MSPIKE_H4=-0.01, RCVD_IN_MSPIKE_WL=-0.01, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Gavin Shan Cc: pbonzini@redhat.com Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" On 3/22/21 8:15 AM, Richard Henderson wrote: > In bbc17caf81f, we used an alias attribute to allow target_page > to be declared const, and yet be initialized late. > > This fails when using LTO with several versions of gcc. > The compiler looks through the alias and decides that the const > variable is statically initialized to zero, then propagates that > zero to many uses of the variable. > > This can be avoided by compiling one object file with -fno-lto. > In this way, any initializer cannot be seen, and the constant > propagation does not occur. > > Since are certain to have this separate compilation unit, we can > drop the alias attribute as well. We simply have differing > declarations for target_page in different compilation units. > Drop the use of init_target_page, and drop the configure detection > for CONFIG_ATTRIBUTE_ALIAS. > > In order to change the compilation flags for a file with meson, > we must use a static_library. This runs into specific_ss, where > we would need to create many static_library instances. > > Fix this by splitting exec-page.c: the page-vary-common.c part is > compiled once as a static_library, while the page-vary.c part is > left in specific_ss in order to handle the target-specific value > of TARGET_PAGE_BITS_MIN. > > Reported-by: Gavin Shan > Signed-off-by: Richard Henderson > --- > configure | 19 ------- > meson.build | 18 ++++++- > include/exec/cpu-all.h | 15 ++---- > include/exec/page-vary.h | 34 ++++++++++++ > exec-vary.c | 108 --------------------------------------- > page-vary-common.c | 54 ++++++++++++++++++++ > page-vary.c | 41 +++++++++++++++ > 7 files changed, 150 insertions(+), 139 deletions(-) > create mode 100644 include/exec/page-vary.h > delete mode 100644 exec-vary.c > create mode 100644 page-vary-common.c > create mode 100644 page-vary.c > It works for me. With this applied, the migration won't fail because of @target_page.bits: Tested-by: Gavin Shan > diff --git a/configure b/configure > index 847bc4d095..dbb873e09c 100755 > --- a/configure > +++ b/configure > @@ -4889,21 +4889,6 @@ if test "$plugins" = "yes" && > "for this purpose. You can't build with --static." > fi > > -######################################## > -# See if __attribute__((alias)) is supported. > -# This false for Xcode 9, but has been remedied for Xcode 10. > -# Unfortunately, travis uses Xcode 9 by default. > - > -attralias=no > -cat > $TMPC << EOF > -int x = 1; > -extern const int y __attribute__((alias("x"))); > -int main(void) { return 0; } > -EOF > -if compile_prog "" "" ; then > - attralias=yes > -fi > - > ######################################## > # check if getauxval is available. > > @@ -5935,10 +5920,6 @@ if test "$atomic64" = "yes" ; then > echo "CONFIG_ATOMIC64=y" >> $config_host_mak > fi > > -if test "$attralias" = "yes" ; then > - echo "CONFIG_ATTRIBUTE_ALIAS=y" >> $config_host_mak > -fi > - > if test "$getauxval" = "yes" ; then > echo "CONFIG_GETAUXVAL=y" >> $config_host_mak > fi > diff --git a/meson.build b/meson.build > index 5c85a15364..24e8897ba2 100644 > --- a/meson.build > +++ b/meson.build > @@ -1933,7 +1933,6 @@ subdir('softmmu') > > common_ss.add(capstone) > specific_ss.add(files('cpu.c', 'disas.c', 'gdbstub.c'), capstone) > -specific_ss.add(files('exec-vary.c')) > specific_ss.add(when: 'CONFIG_TCG', if_true: files( > 'fpu/softfloat.c', > 'tcg/optimize.c', > @@ -1945,6 +1944,23 @@ specific_ss.add(when: 'CONFIG_TCG', if_true: files( > )) > specific_ss.add(when: 'CONFIG_TCG_INTERPRETER', if_true: files('tcg/tci.c')) > > +# Work around a gcc bug/misfeature wherein constant propagation looks > +# through an alias: > +# https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99696 > +# to guess that a const variable is always zero. Without lto, this is > +# impossible, as the alias is restricted to page-vary-common.c. Indeed, > +# without lto, not even the alias is required -- we simply use different > +# declarations in different compilation units. > +pagevary = files('page-vary-common.c') > +if get_option('b_lto') > + pagevary = static_library('page-vary-common', > + sources: pagevary, > + c_args: ['-fno-lto']) > + pagevary = declare_dependency(link_with: pagevary) > +endif > +common_ss.add(pagevary) > +specific_ss.add(files('page-vary.c')) > + > subdir('backends') > subdir('disas') > subdir('migration') > diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h > index 76443eb11d..d76b0b9e02 100644 > --- a/include/exec/cpu-all.h > +++ b/include/exec/cpu-all.h > @@ -215,22 +215,15 @@ static inline void stl_phys_notdirty(AddressSpace *as, hwaddr addr, uint32_t val > /* page related stuff */ > > #ifdef TARGET_PAGE_BITS_VARY > -typedef struct { > - bool decided; > - int bits; > - target_long mask; > -} TargetPageBits; > -#if defined(CONFIG_ATTRIBUTE_ALIAS) || !defined(IN_EXEC_VARY) > +# include "exec/page-vary.h" > extern const TargetPageBits target_page; > -#else > -extern TargetPageBits target_page; > -#endif > #ifdef CONFIG_DEBUG_TCG > #define TARGET_PAGE_BITS ({ assert(target_page.decided); target_page.bits; }) > -#define TARGET_PAGE_MASK ({ assert(target_page.decided); target_page.mask; }) > +#define TARGET_PAGE_MASK ({ assert(target_page.decided); \ > + (target_long)target_page.mask; }) > #else > #define TARGET_PAGE_BITS target_page.bits > -#define TARGET_PAGE_MASK target_page.mask > +#define TARGET_PAGE_MASK ((target_long)target_page.mask) > #endif > #define TARGET_PAGE_SIZE (-(int)TARGET_PAGE_MASK) > #else > diff --git a/include/exec/page-vary.h b/include/exec/page-vary.h > new file mode 100644 > index 0000000000..c22a7a742e > --- /dev/null > +++ b/include/exec/page-vary.h > @@ -0,0 +1,34 @@ > +/* > + * Definitions for cpus with variable page sizes. > + * > + * Copyright (c) 2003 Fabrice Bellard > + * > + * This library is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * This library 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 > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with this library; if not, see . > + */ > + > +#ifndef EXEC_PAGE_VARY_H > +#define EXEC_PAGE_VARY_H > + > +typedef struct { > + bool decided; > + int bits; > + uint64_t mask; > +} TargetPageBits; > + > +#ifdef IN_PAGE_VARY > +extern bool set_preferred_target_page_bits_common(int bits); > +extern void finalize_target_page_bits_common(int min); > +#endif > + > +#endif /* EXEC_PAGE_VARY_H */ > diff --git a/exec-vary.c b/exec-vary.c > deleted file mode 100644 > index a603b1b433..0000000000 > --- a/exec-vary.c > +++ /dev/null > @@ -1,108 +0,0 @@ > -/* > - * Variable page size handling > - * > - * Copyright (c) 2003 Fabrice Bellard > - * > - * This library is free software; you can redistribute it and/or > - * modify it under the terms of the GNU Lesser General Public > - * License as published by the Free Software Foundation; either > - * version 2.1 of the License, or (at your option) any later version. > - * > - * This library 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 > - * Lesser General Public License for more details. > - * > - * You should have received a copy of the GNU Lesser General Public > - * License along with this library; if not, see . > - */ > - > -#include "qemu/osdep.h" > -#include "qemu-common.h" > - > -#define IN_EXEC_VARY 1 > - > -#include "exec/exec-all.h" > - > -#ifdef TARGET_PAGE_BITS_VARY > -# ifdef CONFIG_ATTRIBUTE_ALIAS > -/* > - * We want to declare the "target_page" variable as const, which tells > - * the compiler that it can cache any value that it reads across calls. > - * This avoids multiple assertions and multiple reads within any one user. > - * > - * This works because we finish initializing the data before we ever read > - * from the "target_page" symbol. > - * > - * This also requires that we have a non-constant symbol by which we can > - * perform the actual initialization, and which forces the data to be > - * allocated within writable memory. Thus "init_target_page", and we use > - * that symbol exclusively in the two functions that initialize this value. > - * > - * The "target_page" symbol is created as an alias of "init_target_page". > - */ > -static TargetPageBits init_target_page; > - > -/* > - * Note that this is *not* a redundant decl, this is the definition of > - * the "target_page" symbol. The syntax for this definition requires > - * the use of the extern keyword. This seems to be a GCC bug in > - * either the syntax for the alias attribute or in -Wredundant-decls. > - * > - * See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91765 > - */ > -# pragma GCC diagnostic push > -# pragma GCC diagnostic ignored "-Wredundant-decls" > - > -extern const TargetPageBits target_page > - __attribute__((alias("init_target_page"))); > - > -# pragma GCC diagnostic pop > -# else > -/* > - * When aliases are not supported then we force two different declarations, > - * by way of suppressing the header declaration with IN_EXEC_VARY. > - * We assume that on such an old compiler, LTO cannot be used, and so the > - * compiler cannot not detect the mismatched declarations, and all is well. > - */ > -TargetPageBits target_page; > -# define init_target_page target_page > -# endif > -#endif > - > -bool set_preferred_target_page_bits(int bits) > -{ > - /* > - * The target page size is the lowest common denominator for all > - * the CPUs in the system, so we can only make it smaller, never > - * larger. And we can't make it smaller once we've committed to > - * a particular size. > - */ > -#ifdef TARGET_PAGE_BITS_VARY > - assert(bits >= TARGET_PAGE_BITS_MIN); > - if (init_target_page.bits == 0 || init_target_page.bits > bits) { > - if (init_target_page.decided) { > - return false; > - } > - init_target_page.bits = bits; > - } > -#endif > - return true; > -} > - > -void finalize_target_page_bits(void) > -{ > -#ifdef TARGET_PAGE_BITS_VARY > - if (init_target_page.bits == 0) { > - init_target_page.bits = TARGET_PAGE_BITS_MIN; > - } > - init_target_page.mask = (target_long)-1 << init_target_page.bits; > - init_target_page.decided = true; > - > - /* > - * For the benefit of an -flto build, prevent the compiler from > - * hoisting a read from target_page before we finish initializing. > - */ > - barrier(); > -#endif > -} > diff --git a/page-vary-common.c b/page-vary-common.c > new file mode 100644 > index 0000000000..9175556498 > --- /dev/null > +++ b/page-vary-common.c > @@ -0,0 +1,54 @@ > +/* > + * Variable page size handling -- target independent part. > + * > + * Copyright (c) 2003 Fabrice Bellard > + * > + * This library is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * This library 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 > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with this library; if not, see . > + */ > + > +#define IN_PAGE_VARY 1 > + > +#include "qemu/osdep.h" > +#include "qemu-common.h" > +#include "exec/page-vary.h" > + > +/* WARNING: This file must *not* be complied with -flto. */ > + > +TargetPageBits target_page; > + > +bool set_preferred_target_page_bits_common(int bits) > +{ > + /* > + * The target page size is the lowest common denominator for all > + * the CPUs in the system, so we can only make it smaller, never > + * larger. And we can't make it smaller once we've committed to > + * a particular size. > + */ > + if (target_page.bits == 0 || target_page.bits > bits) { > + if (target_page.decided) { > + return false; > + } > + target_page.bits = bits; > + } > + return true; > +} > + > +void finalize_target_page_bits_common(int min) > +{ > + if (target_page.bits == 0) { > + target_page.bits = min; > + } > + target_page.mask = -1ull << target_page.bits; > + target_page.decided = true; > +} > diff --git a/page-vary.c b/page-vary.c > new file mode 100644 > index 0000000000..057c7f1815 > --- /dev/null > +++ b/page-vary.c > @@ -0,0 +1,41 @@ > +/* > + * Variable page size handling -- target specific part. > + * > + * Copyright (c) 2003 Fabrice Bellard > + * > + * This library is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * This library 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 > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with this library; if not, see . > + */ > + > +#define IN_PAGE_VARY 1 > + > +#include "qemu/osdep.h" > +#include "qemu-common.h" > +#include "exec/exec-all.h" > + > +bool set_preferred_target_page_bits(int bits) > +{ > +#ifdef TARGET_PAGE_BITS_VARY > + assert(bits >= TARGET_PAGE_BITS_MIN); > + return set_preferred_target_page_bits_common(bits); > +#else > + return true; > +#endif > +} > + > +void finalize_target_page_bits(void) > +{ > +#ifdef TARGET_PAGE_BITS_VARY > + finalize_target_page_bits_common(TARGET_PAGE_BITS_MIN); > +#endif > +} >