From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 2/2] x86/mtrr: fix build with gcc9 Date: Fri, 15 Mar 2019 16:21:38 +0000 Message-ID: <7520f3cc-1b62-4616-9f93-e061357efa78@citrix.com> References: <5C80F130020000780021C602@prv1-mh.provo.novell.com> <5C80F32C020000780021C626@prv1-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4272493776245233758==" Return-path: Received: from all-amaz-eas1.inumbo.com ([34.197.232.57] helo=us1-amaz-eas2.inumbo.com) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1h4pt8-00013l-0X for xen-devel@lists.xenproject.org; Fri, 15 Mar 2019 16:40:54 +0000 In-Reply-To: <5C80F32C020000780021C626@prv1-mh.provo.novell.com> Content-Language: en-GB List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xenproject.org Sender: "Xen-devel" To: Jan Beulich , xen-devel Cc: Charles Arnold , Juergen Gross , Wei Liu , Roger Pau Monne List-Id: xen-devel@lists.xenproject.org --===============4272493776245233758== Content-Type: multipart/alternative; boundary="------------C25D6C616C74E039AAFEC904" Content-Language: en-GB --------------C25D6C616C74E039AAFEC904 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit On 07/03/2019 10:32, Jan Beulich wrote: > generic.c: In function ‘print_mtrr_state’: > generic.c:210:11: error: ‘%0*lx’ directive output between 1 and 1073741823 bytes may cause result to exceed > ‘INT_MAX’ [-Werror=format-overflow=] > 210 | printk("%s %u base %0*"PRIx64"000 mask %0*"PRIx64"000 %s\n", > | ^~~~~~~~~~~~~~~~~ > generic.c:210:44: note: format string is defined here > 210 | printk("%s %u base %0*"PRIx64"000 mask %0*"PRIx64"000 %s\n", > generic.c:210:11: note: directive argument in the range [0, 4503599627370495] > 210 | printk("%s %u base %0*"PRIx64"000 mask %0*"PRIx64"000 %s\n", > | ^~~~~~~~~~~~~~~~~ > generic.c:210:11: note: assuming directive output of 1 byte > > Restrict the width of the variable "width" controlling the number of > address digits output. > > Reported-by: Charles Arnold > Signed-off-by: Jan Beulich This is because GCC doesn't know the value of paddr_bits, and has concluded that width = (paddr_bits - PAGE_SHIFT + 3) / 4; can result in some insane values, which is true. However, this logic to unnecessarily complicated for something which is only printed in a verbose or error case. I'd prefer this as an alternative: diff --git a/xen/arch/x86/cpu/mtrr/generic.c b/xen/arch/x86/cpu/mtrr/generic.c index 8f9cf1b..566396f 100644 --- a/xen/arch/x86/cpu/mtrr/generic.c +++ b/xen/arch/x86/cpu/mtrr/generic.c @@ -182,7 +182,6 @@ static void __init print_fixed(unsigned int base, unsigned int step, static void __init print_mtrr_state(const char *level) { unsigned int i; - int width; printk("%sMTRR default type: %s\n", level, mtrr_attrib_to_str(mtrr_state.def_type)); @@ -203,14 +202,13 @@ static void __init print_mtrr_state(const char *level) } printk("%sMTRR variable ranges %sabled:\n", level, mtrr_state.enabled ? "en" : "dis"); - width = (paddr_bits - PAGE_SHIFT + 3) / 4; for (i = 0; i < num_var_ranges; ++i) { if (mtrr_state.var_ranges[i].mask & MTRR_PHYSMASK_VALID) - printk("%s %u base %0*"PRIx64"000 mask %0*"PRIx64"000 %s\n", + printk("%s %u base %013"PRIx64"000 mask %013"PRIx64"000 %s\n", level, i, - width, mtrr_state.var_ranges[i].base >> 12, - width, mtrr_state.var_ranges[i].mask >> 12, + mtrr_state.var_ranges[i].base >> 12, + mtrr_state.var_ranges[i].mask >> 12, mtrr_attrib_to_str(mtrr_state.var_ranges[i].base & MTRR_PHYSBASE_TYPE_MASK)); else --------------C25D6C616C74E039AAFEC904 Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: 8bit
On 07/03/2019 10:32, Jan Beulich wrote:
generic.c: In function ‘print_mtrr_state’:
generic.c:210:11: error: ‘%0*lx’ directive output between 1 and 1073741823 bytes may cause result to exceed
‘INT_MAX’ [-Werror=format-overflow=]
  210 |    printk("%s  %u base %0*"PRIx64"000 mask %0*"PRIx64"000 %s\n",
      |           ^~~~~~~~~~~~~~~~~
generic.c:210:44: note: format string is defined here
  210 |    printk("%s  %u base %0*"PRIx64"000 mask %0*"PRIx64"000 %s\n",
generic.c:210:11: note: directive argument in the range [0, 4503599627370495]
  210 |    printk("%s  %u base %0*"PRIx64"000 mask %0*"PRIx64"000 %s\n",
      |           ^~~~~~~~~~~~~~~~~
generic.c:210:11: note: assuming directive output of 1 byte

Restrict the width of the variable "width" controlling the number of
address digits output.

Reported-by: Charles Arnold <carnold@suse.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

This is because GCC doesn't know the value of paddr_bits, and has concluded that

width = (paddr_bits - PAGE_SHIFT + 3) / 4;

can result in some insane values, which is true.

However, this logic to unnecessarily complicated for something which is only printed in a verbose or error case.

I'd prefer this as an alternative:

diff --git a/xen/arch/x86/cpu/mtrr/generic.c b/xen/arch/x86/cpu/mtrr/generic.c
index 8f9cf1b..566396f 100644
--- a/xen/arch/x86/cpu/mtrr/generic.c
+++ b/xen/arch/x86/cpu/mtrr/generic.c
@@ -182,7 +182,6 @@ static void __init print_fixed(unsigned int base, unsigned int step,
 static void __init print_mtrr_state(const char *level)
 {
        unsigned int i;
-       int width;
 
        printk("%sMTRR default type: %s\n", level,
               mtrr_attrib_to_str(mtrr_state.def_type));
@@ -203,14 +202,13 @@ static void __init print_mtrr_state(const char *level)
        }
        printk("%sMTRR variable ranges %sabled:\n", level,
               mtrr_state.enabled ? "en" : "dis");
-       width = (paddr_bits - PAGE_SHIFT + 3) / 4;
 
        for (i = 0; i < num_var_ranges; ++i) {
                if (mtrr_state.var_ranges[i].mask & MTRR_PHYSMASK_VALID)
-                       printk("%s  %u base %0*"PRIx64"000 mask %0*"PRIx64"000 %s\n",
+                       printk("%s  %u base %013"PRIx64"000 mask %013"PRIx64"000 %s\n",
                               level, i,
-                              width, mtrr_state.var_ranges[i].base >> 12,
-                              width, mtrr_state.var_ranges[i].mask >> 12,
+                              mtrr_state.var_ranges[i].base >> 12,
+                              mtrr_state.var_ranges[i].mask >> 12,
                               mtrr_attrib_to_str(mtrr_state.var_ranges[i].base &
                                                  MTRR_PHYSBASE_TYPE_MASK));
                else
--------------C25D6C616C74E039AAFEC904-- --===============4272493776245233758== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVucHJvamVjdC5vcmcKaHR0cHM6Ly9saXN0 cy54ZW5wcm9qZWN0Lm9yZy9tYWlsbWFuL2xpc3RpbmZvL3hlbi1kZXZlbA== --===============4272493776245233758==--