* [PATCH 0/2] x86: fix build with gcc9 @ 2019-03-07 10:23 Jan Beulich 2019-03-07 10:31 ` [PATCH 1/2] x86/e820: " Jan Beulich ` (4 more replies) 0 siblings, 5 replies; 22+ messages in thread From: Jan Beulich @ 2019-03-07 10:23 UTC (permalink / raw) To: xen-devel Cc: Charles Arnold, Andrew Cooper, Wei Liu, Juergen Gross, Roger Pau Monne Several build issues (new warnings, causing the build to fail due to -Werror) have been found. The two changes here address the ones I can actually repro; there are a few more related to -Waddress-of-packed-member which I haven't been able to see myself, and hence for now I'm unable to sort out a proper fix / workaround for them. Despite this coming late, I think this is a worthwhile change for 4.12 with pretty low risk. 1: e820: fix build with gcc9 2: mtrr: fix build with gcc9 Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/2] x86/e820: fix build with gcc9 2019-03-07 10:23 [PATCH 0/2] x86: fix build with gcc9 Jan Beulich @ 2019-03-07 10:31 ` Jan Beulich 2019-03-07 10:46 ` Roger Pau Monné 2019-03-15 16:07 ` Andrew Cooper 2019-03-07 10:32 ` [PATCH 2/2] x86/mtrr: " Jan Beulich ` (3 subsequent siblings) 4 siblings, 2 replies; 22+ messages in thread From: Jan Beulich @ 2019-03-07 10:31 UTC (permalink / raw) To: xen-devel Cc: Charles Arnold, Andrew Cooper, Wei Liu, Juergen Gross, Roger Pau Monne e820.c: In function ‘clip_to_limit’: .../xen/include/asm/string.h:10:26: error: ‘__builtin_memmove’ offset [-16, -36] is out of the bounds [0, 20484] of object ‘e820’ with type ‘struct e820map’ [-Werror=array-bounds] 10 | #define memmove(d, s, n) __builtin_memmove(d, s, n) | ^~~~~~~~~~~~~~~~~~~~~~~~~~ e820.c:404:13: note: in expansion of macro ‘memmove’ 404 | memmove(&e820.map[i], &e820.map[i+1], | ^~~~~~~ e820.c:36:16: note: ‘e820’ declared here 36 | struct e820map e820; | ^~~~ While I can't see where the negative offsets would come from, converting the loop index to unsigned type helps. Take the opportunity and also convert several other local variables and copy_e820_map()'s second parameter to unsigned int (and bool in one case). Reported-by: Charles Arnold <carnold@suse.com> Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/e820.c +++ b/xen/arch/x86/e820.c @@ -44,7 +44,7 @@ struct e820map __initdata e820_raw; */ int __init e820_all_mapped(u64 start, u64 end, unsigned type) { - int i; + unsigned int i; for (i = 0; i < e820.nr_map; i++) { struct e820entry *ei = &e820.map[i]; @@ -73,9 +73,7 @@ int __init e820_all_mapped(u64 start, u6 static void __init add_memory_region(unsigned long long start, unsigned long long size, int type) { - int x; - - x = e820.nr_map; + unsigned int x = e820.nr_map; if (x == ARRAY_SIZE(e820.map)) { printk(KERN_ERR "Ooops! Too many entries in the memory map!\n"); @@ -140,11 +138,9 @@ int __init sanitize_e820_map(struct e820 struct change_member *change_tmp; unsigned long current_type, last_type; unsigned long long last_addr; - int chgidx, still_changing; - int overlap_entries; - int new_bios_entry; - int old_nr, new_nr, chg_nr; - int i; + bool still_changing; + unsigned int i, chgidx, overlap_entries, new_bios_entry; + unsigned int old_nr, new_nr, chg_nr; /* Visually we're performing the following (1,2,3,4 = memory types)... @@ -211,9 +207,9 @@ int __init sanitize_e820_map(struct e820 chg_nr = chgidx; /* true number of change-points */ /* sort change-point list by memory addresses (low -> high) */ - still_changing = 1; + still_changing = true; while (still_changing) { - still_changing = 0; + still_changing = false; for (i=1; i < chg_nr; i++) { /* if <current_addr> > <last_addr>, swap */ /* or, if current=<start_addr> & last=<end_addr>, swap */ @@ -226,7 +222,7 @@ int __init sanitize_e820_map(struct e820 change_tmp = change_point[i]; change_point[i] = change_point[i-1]; change_point[i-1] = change_tmp; - still_changing=1; + still_changing = true; } } } @@ -304,7 +300,7 @@ int __init sanitize_e820_map(struct e820 * thinkpad 560x, for example, does not cooperate with the memory * detection code.) */ -static int __init copy_e820_map(struct e820entry * biosmap, int nr_map) +static int __init copy_e820_map(struct e820entry * biosmap, unsigned int nr_map) { /* Only one memory region (or negative)? Ignore it */ if (nr_map < 2) @@ -345,7 +341,7 @@ static int __init copy_e820_map(struct e */ static unsigned long __init find_max_pfn(void) { - int i; + unsigned int i; unsigned long max_pfn = 0; for (i = 0; i < e820.nr_map; i++) { @@ -366,7 +362,7 @@ static unsigned long __init find_max_pfn static void __init clip_to_limit(uint64_t limit, char *warnmsg) { - int i; + unsigned int i; char _warnmsg[160]; uint64_t old_limit = 0; @@ -514,7 +510,7 @@ static void __init machine_specific_memo { unsigned long mpt_limit, ro_mpt_limit; uint64_t top_of_ram, size; - int i; + unsigned int i; sanitize_e820_map(raw->map, &raw->nr_map); copy_e820_map(raw->map, raw->nr_map); @@ -604,7 +600,7 @@ int __init e820_change_range_type( uint32_t orig_type, uint32_t new_type) { uint64_t rs = 0, re = 0; - int i; + unsigned int i; for ( i = 0; i < e820->nr_map; i++ ) { _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] x86/e820: fix build with gcc9 2019-03-07 10:31 ` [PATCH 1/2] x86/e820: " Jan Beulich @ 2019-03-07 10:46 ` Roger Pau Monné 2019-03-07 10:55 ` Wei Liu 2019-03-15 16:07 ` Andrew Cooper 1 sibling, 1 reply; 22+ messages in thread From: Roger Pau Monné @ 2019-03-07 10:46 UTC (permalink / raw) To: Jan Beulich Cc: Charles Arnold, xen-devel, Wei Liu, Juergen Gross, Andrew Cooper On Thu, Mar 07, 2019 at 03:31:43AM -0700, Jan Beulich wrote: > e820.c: In function ‘clip_to_limit’: > .../xen/include/asm/string.h:10:26: error: ‘__builtin_memmove’ offset [-16, -36] is out of the bounds [0, 20484] of > object ‘e820’ with type ‘struct e820map’ [-Werror=array-bounds] > 10 | #define memmove(d, s, n) __builtin_memmove(d, s, n) > | ^~~~~~~~~~~~~~~~~~~~~~~~~~ > e820.c:404:13: note: in expansion of macro ‘memmove’ > 404 | memmove(&e820.map[i], &e820.map[i+1], > | ^~~~~~~ > e820.c:36:16: note: ‘e820’ declared here > 36 | struct e820map e820; > | ^~~~ > > While I can't see where the negative offsets would come from, converting > the loop index to unsigned type helps. Take the opportunity and also > convert several other local variables and copy_e820_map()'s second > parameter to unsigned int (and bool in one case). I also cannot see how you can end up with negative offsets, but changing to unsigned int is definitely and improvement IMO. > Reported-by: Charles Arnold <carnold@suse.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] x86/e820: fix build with gcc9 2019-03-07 10:46 ` Roger Pau Monné @ 2019-03-07 10:55 ` Wei Liu 0 siblings, 0 replies; 22+ messages in thread From: Wei Liu @ 2019-03-07 10:55 UTC (permalink / raw) To: Roger Pau Monné Cc: Charles Arnold, Juergen Gross, Wei Liu, Andrew Cooper, Jan Beulich, xen-devel On Thu, Mar 07, 2019 at 11:46:08AM +0100, Roger Pau Monné wrote: > On Thu, Mar 07, 2019 at 03:31:43AM -0700, Jan Beulich wrote: > > e820.c: In function ‘clip_to_limit’: > > .../xen/include/asm/string.h:10:26: error: ‘__builtin_memmove’ offset [-16, -36] is out of the bounds [0, 20484] of > > object ‘e820’ with type ‘struct e820map’ [-Werror=array-bounds] > > 10 | #define memmove(d, s, n) __builtin_memmove(d, s, n) > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~ > > e820.c:404:13: note: in expansion of macro ‘memmove’ > > 404 | memmove(&e820.map[i], &e820.map[i+1], > > | ^~~~~~~ > > e820.c:36:16: note: ‘e820’ declared here > > 36 | struct e820map e820; > > | ^~~~ > > > > While I can't see where the negative offsets would come from, converting > > the loop index to unsigned type helps. Take the opportunity and also > > convert several other local variables and copy_e820_map()'s second > > parameter to unsigned int (and bool in one case). > > I also cannot see how you can end up with negative offsets, but > changing to unsigned int is definitely and improvement IMO. Gcc is becoming more and more aggressive in inferring what you mean with your code. There are definitely bugs in that area. This one comes to mind: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86827 Although I'm not sure if -Warray-bounds is the culprit in this case. > > > Reported-by: Charles Arnold <carnold@suse.com> > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > > Thanks. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] x86/e820: fix build with gcc9 2019-03-07 10:31 ` [PATCH 1/2] x86/e820: " Jan Beulich 2019-03-07 10:46 ` Roger Pau Monné @ 2019-03-15 16:07 ` Andrew Cooper 2019-03-18 10:00 ` Jan Beulich 1 sibling, 1 reply; 22+ messages in thread From: Andrew Cooper @ 2019-03-15 16:07 UTC (permalink / raw) To: Jan Beulich, xen-devel Cc: Charles Arnold, Juergen Gross, Wei Liu, Roger Pau Monne On 07/03/2019 10:31, Jan Beulich wrote: > e820.c: In function ‘clip_to_limit’: > .../xen/include/asm/string.h:10:26: error: ‘__builtin_memmove’ offset [-16, -36] is out of the bounds [0, 20484] of > object ‘e820’ with type ‘struct e820map’ [-Werror=array-bounds] > 10 | #define memmove(d, s, n) __builtin_memmove(d, s, n) > | ^~~~~~~~~~~~~~~~~~~~~~~~~~ > e820.c:404:13: note: in expansion of macro ‘memmove’ > 404 | memmove(&e820.map[i], &e820.map[i+1], > | ^~~~~~~ > e820.c:36:16: note: ‘e820’ declared here > 36 | struct e820map e820; > | ^~~~ > > While I can't see where the negative offsets would come from, converting > the loop index to unsigned type helps. Take the opportunity and also > convert several other local variables and copy_e820_map()'s second > parameter to unsigned int (and bool in one case). > > Reported-by: Charles Arnold <carnold@suse.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>, with one request. > > --- a/xen/arch/x86/e820.c > +++ b/xen/arch/x86/e820.c > @@ -304,7 +300,7 @@ int __init sanitize_e820_map(struct e820 > * thinkpad 560x, for example, does not cooperate with the memory > * detection code.) > */ > -static int __init copy_e820_map(struct e820entry * biosmap, int nr_map) > +static int __init copy_e820_map(struct e820entry * biosmap, unsigned int nr_map) > { > /* Only one memory region (or negative)? Ignore it */ This comment is now stale. I'd just drop the bit in brackets. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] x86/e820: fix build with gcc9 2019-03-15 16:07 ` Andrew Cooper @ 2019-03-18 10:00 ` Jan Beulich 0 siblings, 0 replies; 22+ messages in thread From: Jan Beulich @ 2019-03-18 10:00 UTC (permalink / raw) To: Andrew Cooper, xen-devel Cc: Charles Arnold, Juergen Gross, Wei Liu, Roger Pau Monne >>> On 15.03.19 at 17:07, <andrew.cooper3@citrix.com> wrote: > On 07/03/2019 10:31, Jan Beulich wrote: >> e820.c: In function ‘clip_to_limit’: >> .../xen/include/asm/string.h:10:26: error: ‘__builtin_memmove’ offset [-16, > -36] is out of the bounds [0, 20484] of >> object ‘e820’ with type ‘struct e820map’ [-Werror=array-bounds] >> 10 | #define memmove(d, s, n) __builtin_memmove(d, s, n) >> | ^~~~~~~~~~~~~~~~~~~~~~~~~~ >> e820.c:404:13: note: in expansion of macro ‘memmove’ >> 404 | memmove(&e820.map[i], &e820.map[i+1], >> | ^~~~~~~ >> e820.c:36:16: note: ‘e820’ declared here >> 36 | struct e820map e820; >> | ^~~~ >> >> While I can't see where the negative offsets would come from, converting >> the loop index to unsigned type helps. Take the opportunity and also >> convert several other local variables and copy_e820_map()'s second >> parameter to unsigned int (and bool in one case). >> >> Reported-by: Charles Arnold <carnold@suse.com> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>, with one request. Thanks. >> --- a/xen/arch/x86/e820.c >> +++ b/xen/arch/x86/e820.c >> @@ -304,7 +300,7 @@ int __init sanitize_e820_map(struct e820 >> * thinkpad 560x, for example, does not cooperate with the memory >> * detection code.) >> */ >> -static int __init copy_e820_map(struct e820entry * biosmap, int nr_map) >> +static int __init copy_e820_map(struct e820entry * biosmap, unsigned int nr_map) >> { >> /* Only one memory region (or negative)? Ignore it */ > > This comment is now stale. I'd just drop the bit in brackets. Sure, will do. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/2] x86/mtrr: fix build with gcc9 2019-03-07 10:23 [PATCH 0/2] x86: fix build with gcc9 Jan Beulich 2019-03-07 10:31 ` [PATCH 1/2] x86/e820: " Jan Beulich @ 2019-03-07 10:32 ` Jan Beulich 2019-03-07 10:55 ` Roger Pau Monné ` (2 more replies) 2019-03-07 11:12 ` [PATCH 0/2] x86: " Wei Liu ` (2 subsequent siblings) 4 siblings, 3 replies; 22+ messages in thread From: Jan Beulich @ 2019-03-07 10:32 UTC (permalink / raw) To: xen-devel Cc: Charles Arnold, Andrew Cooper, Wei Liu, Juergen Gross, Roger Pau Monne 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> --- a/xen/arch/x86/cpu/mtrr/generic.c +++ b/xen/arch/x86/cpu/mtrr/generic.c @@ -182,7 +182,7 @@ static void __init print_fixed(unsigned static void __init print_mtrr_state(const char *level) { unsigned int i; - int width; + unsigned char width; /* gcc9 doesn't like plain "int" here */ printk("%sMTRR default type: %s\n", level, mtrr_attrib_to_str(mtrr_state.def_type)); _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] x86/mtrr: fix build with gcc9 2019-03-07 10:32 ` [PATCH 2/2] x86/mtrr: " Jan Beulich @ 2019-03-07 10:55 ` Roger Pau Monné 2019-03-07 11:22 ` Jan Beulich 2019-03-15 16:21 ` Andrew Cooper [not found] ` <5C80F32C0200000000103FF7@prv1-mh.provo.novell.com> 2 siblings, 1 reply; 22+ messages in thread From: Roger Pau Monné @ 2019-03-07 10:55 UTC (permalink / raw) To: Jan Beulich Cc: Charles Arnold, xen-devel, Wei Liu, Juergen Gross, Andrew Cooper On Thu, Mar 07, 2019 at 03:32:12AM -0700, 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. I have to admit I'm not sure why gcc complains here, and why switching to unsigned char fixes it. unsigned char max value would be 255, which when used as a width to print an unsigned long it's also too high? Does checking that width <= 16 also placate gcc? Or maybe I'm just not understanding what gcc complains about. Thanks. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] x86/mtrr: fix build with gcc9 2019-03-07 10:55 ` Roger Pau Monné @ 2019-03-07 11:22 ` Jan Beulich 2019-03-07 14:20 ` Roger Pau Monné 0 siblings, 1 reply; 22+ messages in thread From: Jan Beulich @ 2019-03-07 11:22 UTC (permalink / raw) To: Roger Pau Monne Cc: Charles Arnold, Andrew Cooper, Wei Liu, Juergen Gross, xen-devel >>> On 07.03.19 at 11:55, <roger.pau@citrix.com> wrote: > On Thu, Mar 07, 2019 at 03:32:12AM -0700, 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. > > I have to admit I'm not sure why gcc complains here, and why switching > to unsigned char fixes it. unsigned char max value would be 255, which > when used as a width to print an unsigned long it's also too high? With width = (paddr_bits - PAGE_SHIFT + 3) / 4; gcc simply can't know that the valid value range is rather small. Hence it assumes millions of leading zeros _could_ be printed. Their doc explicitly suggests using a more narrow type, which is why ... > Does checking that width <= 16 also placate gcc? ... I didn't even try this option (despite having considered it). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] x86/mtrr: fix build with gcc9 2019-03-07 11:22 ` Jan Beulich @ 2019-03-07 14:20 ` Roger Pau Monné 0 siblings, 0 replies; 22+ messages in thread From: Roger Pau Monné @ 2019-03-07 14:20 UTC (permalink / raw) To: Jan Beulich Cc: Charles Arnold, Andrew Cooper, Wei Liu, Juergen Gross, xen-devel On Thu, Mar 07, 2019 at 04:22:13AM -0700, Jan Beulich wrote: > >>> On 07.03.19 at 11:55, <roger.pau@citrix.com> wrote: > > On Thu, Mar 07, 2019 at 03:32:12AM -0700, 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. > > > > I have to admit I'm not sure why gcc complains here, and why switching > > to unsigned char fixes it. unsigned char max value would be 255, which > > when used as a width to print an unsigned long it's also too high? > > With > > width = (paddr_bits - PAGE_SHIFT + 3) / 4; > > gcc simply can't know that the valid value range is rather small. Hence > it assumes millions of leading zeros _could_ be printed. Their doc > explicitly suggests using a more narrow type, which is why ... > > > Does checking that width <= 16 also placate gcc? > > ... I didn't even try this option (despite having considered it). Ack, if that's the recommended way. I would prefer using unsigned int and adding a BUG_ON if > 16. Anyway: Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] x86/mtrr: fix build with gcc9 2019-03-07 10:32 ` [PATCH 2/2] x86/mtrr: " Jan Beulich 2019-03-07 10:55 ` Roger Pau Monné @ 2019-03-15 16:21 ` Andrew Cooper 2019-03-18 10:11 ` Jan Beulich [not found] ` <5C80F32C0200000000103FF7@prv1-mh.provo.novell.com> 2 siblings, 1 reply; 22+ messages in thread From: Andrew Cooper @ 2019-03-15 16:21 UTC (permalink / raw) To: Jan Beulich, xen-devel Cc: Charles Arnold, Juergen Gross, Wei Liu, Roger Pau Monne [-- Attachment #1.1: Type: text/plain, Size: 2808 bytes --] 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 [-- Attachment #1.2: Type: text/html, Size: 3439 bytes --] [-- Attachment #2: Type: text/plain, Size: 157 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] x86/mtrr: fix build with gcc9 2019-03-15 16:21 ` Andrew Cooper @ 2019-03-18 10:11 ` Jan Beulich 2019-03-18 10:30 ` Andrew Cooper 0 siblings, 1 reply; 22+ messages in thread From: Jan Beulich @ 2019-03-18 10:11 UTC (permalink / raw) To: Andrew Cooper Cc: Charles Arnold, xen-devel, Wei Liu, Juergen Gross, Roger Pau Monne >>> On 15.03.19 at 17:21, <andrew.cooper3@citrix.com> wrote: > 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, I don't prefer this, and it was done the way it is for a very simple reason: By omitting unnecessary leading zeros we convey an extra bit of information - this way it is easier to spot if the mask values in particular indeed go up all the way to the paddr limit. I have at least one system where the BIOS screws up, and there end up being leading zeros. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] x86/mtrr: fix build with gcc9 2019-03-18 10:11 ` Jan Beulich @ 2019-03-18 10:30 ` Andrew Cooper 2019-03-18 10:53 ` Jan Beulich 0 siblings, 1 reply; 22+ messages in thread From: Andrew Cooper @ 2019-03-18 10:30 UTC (permalink / raw) To: Jan Beulich Cc: Charles Arnold, xen-devel, Wei Liu, Juergen Gross, Roger Pau Monne On 18/03/2019 10:11, Jan Beulich wrote: >>>> On 15.03.19 at 17:21, <andrew.cooper3@citrix.com> wrote: >> 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, > I don't prefer this, and it was done the way it is for a very simple > reason: By omitting unnecessary leading zeros we convey an > extra bit of information - this way it is easier to spot if the mask > values in particular indeed go up all the way to the paddr limit. I > have at least one system where the BIOS screws up, and there > end up being leading zeros. How is that expected to work in the way you describe for the overwhelming majority of systems with don't have MAXPHYSADDR aligned on a nibble? 39, 42 and 46 are the widths used in practice by Intel processors, none of which are divisible by 4. If you want to print this information out in a useful way, print 1ul << paddr_bits so it is obvious in the logs. Your logic of omitting leading zeros is of no use to the Xen community when you are the only person who knows what it means. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] x86/mtrr: fix build with gcc9 2019-03-18 10:30 ` Andrew Cooper @ 2019-03-18 10:53 ` Jan Beulich 0 siblings, 0 replies; 22+ messages in thread From: Jan Beulich @ 2019-03-18 10:53 UTC (permalink / raw) To: Andrew Cooper Cc: Charles Arnold, xen-devel, Wei Liu, Juergen Gross, Roger Pau Monne >>> On 18.03.19 at 11:30, <andrew.cooper3@citrix.com> wrote: > On 18/03/2019 10:11, Jan Beulich wrote: >>>>> On 15.03.19 at 17:21, <andrew.cooper3@citrix.com> wrote: >>> @@ -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, >> I don't prefer this, and it was done the way it is for a very simple >> reason: By omitting unnecessary leading zeros we convey an >> extra bit of information - this way it is easier to spot if the mask >> values in particular indeed go up all the way to the paddr limit. I >> have at least one system where the BIOS screws up, and there >> end up being leading zeros. > > How is that expected to work in the way you describe for the > overwhelming majority of systems with don't have MAXPHYSADDR aligned on > a nibble? > > 39, 42 and 46 are the widths used in practice by Intel processors, none > of which are divisible by 4. I didn't say it's ideal, but as said - on one of my systems it makes very obvious a flaw in the system's BIOS. I think that's good enough as a reason. > If you want to print this information out in a useful way, print 1ul << > paddr_bits so it is obvious in the logs. Which will require everyone trying to consume this to count zeros and f-s. > Your logic of omitting > leading zeros is of no use to the Xen community when you are the only > person who knows what it means. No-one is prevented from knowing this. In fact, for the purpose of reducing pressure on both serial line bandwidth and log buffer size, I think we'd be better off logging _all_ physical addresses and MFNs with just as many leading zeros as are meaningful on the platform. Granted in several cases even just going from %016lx to %013lx would already be a reduction, but we clearly can do better. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
[parent not found: <5C80F32C0200000000103FF7@prv1-mh.provo.novell.com>]
[parent not found: <5C80F32C0200007800232900@prv1-mh.provo.novell.com>]
[parent not found: <5C80F32C0200000000104D67@prv1-mh.provo.novell.com>]
[parent not found: <5C80F32C0200007800238665@prv1-mh.provo.novell.com>]
* [Xen-devel] Ping: [PATCH 2/2] x86/mtrr: fix build with gcc9 [not found] ` <5C80F32C0200007800238665@prv1-mh.provo.novell.com> @ 2019-06-14 15:56 ` Jan Beulich 2019-06-17 15:47 ` Andrew Cooper 0 siblings, 1 reply; 22+ messages in thread From: Jan Beulich @ 2019-06-14 15:56 UTC (permalink / raw) To: Andrew Cooper; +Cc: Charles Arnold, xen-devel, Wei Liu, Roger Pau Monne >>> On 07.03.19 at 11:32, 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 one's still pending for us to build cleanly with gcc 9. I know you'd like it be done differently, but I'm not happy with the implications of your suggestion, and I've explained why. I would (hesitantly, i.e. just to get the build issue out of the way) ack your variant if you submitted it, but I'd appreciate if you would re-consider whether you could live with going with the one here. Jan > --- a/xen/arch/x86/cpu/mtrr/generic.c > +++ b/xen/arch/x86/cpu/mtrr/generic.c > @@ -182,7 +182,7 @@ static void __init print_fixed(unsigned > static void __init print_mtrr_state(const char *level) > { > unsigned int i; > - int width; > + unsigned char width; /* gcc9 doesn't like plain "int" here */ > > printk("%sMTRR default type: %s\n", level, > mtrr_attrib_to_str(mtrr_state.def_type)); > > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Xen-devel] Ping: [PATCH 2/2] x86/mtrr: fix build with gcc9 2019-06-14 15:56 ` [Xen-devel] Ping: " Jan Beulich @ 2019-06-17 15:47 ` Andrew Cooper 2019-06-17 16:08 ` Jan Beulich 0 siblings, 1 reply; 22+ messages in thread From: Andrew Cooper @ 2019-06-17 15:47 UTC (permalink / raw) To: Jan Beulich; +Cc: Charles Arnold, xen-devel, Wei Liu, Roger Pau Monne On 14/06/2019 16:56, Jan Beulich wrote: >>>> On 07.03.19 at 11:32, 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 one's still pending for us to build cleanly with gcc 9. I can't reproduce it with any build of GCC 9 (all of which are straight from the upstream tree). Is this a locally patched version? > I know you'd like it be done differently, but I'm not happy with the > implications of your suggestion, and I've explained why. But you haven't adequately (IMO) addressed any of the shortcomings. Notably that the argument falls down on all common Intel platforms, and its still a piece of magic which only you know how to interpret. > I would > (hesitantly, i.e. just to get the build issue out of the way) ack > your variant if you submitted it, but I'd appreciate if you would > re-consider whether you could live with going with the one here. I'm happy to put a SoB and real commit message on my patch, but I'd like to actually get to the bottom of the build failure, given that it hasn't been reproduced by anyone else using GCC 9. I don't view limiting the type as a viable fix, because all does is try to game whichever piece of logic GCC is using to object to the construct, and is therefore likely to break again in the future. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Xen-devel] Ping: [PATCH 2/2] x86/mtrr: fix build with gcc9 2019-06-17 15:47 ` Andrew Cooper @ 2019-06-17 16:08 ` Jan Beulich 0 siblings, 0 replies; 22+ messages in thread From: Jan Beulich @ 2019-06-17 16:08 UTC (permalink / raw) To: Andrew Cooper; +Cc: Charles Arnold, xen-devel, Wei Liu, Roger Pau Monne >>> On 17.06.19 at 17:47, <andrew.cooper3@citrix.com> wrote: > On 14/06/2019 16:56, Jan Beulich wrote: >>>>> On 07.03.19 at 11:32, 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 one's still pending for us to build cleanly with gcc 9. > > I can't reproduce it with any build of GCC 9 (all of which are straight > from the upstream tree). Is this a locally patched version? This was with a pre-RC version of gcc9; I wouldn't be surprised if they changed their code before the release. I admit it didn't occur to me to retry building without the patch. >> I know you'd like it be done differently, but I'm not happy with the >> implications of your suggestion, and I've explained why. > > But you haven't adequately (IMO) addressed any of the shortcomings. > Notably that the argument falls down on all common Intel platforms, and > its still a piece of magic which only you know how to interpret. I'm unaware of shortcomings, and the "magics" of the number of digits logged is simply not relevant to people now knowing of this. It is relevant to people like me who do know. And I can't do anything about the number of address bits on common Intel platforms not being evenly divisible by 4. >> I would >> (hesitantly, i.e. just to get the build issue out of the way) ack >> your variant if you submitted it, but I'd appreciate if you would >> re-consider whether you could live with going with the one here. > > I'm happy to put a SoB and real commit message on my patch, but I'd like > to actually get to the bottom of the build failure, given that it hasn't > been reproduced by anyone else using GCC 9. > > I don't view limiting the type as a viable fix, because all does is try > to game whichever piece of logic GCC is using to object to the > construct, and is therefore likely to break again in the future. Well, I can't exclude this happening, but the mention of INT_MAX in the diagnostic doesn't seem to make this very likely with an 8-bit wide width specifier. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/2] x86: fix build with gcc9 2019-03-07 10:23 [PATCH 0/2] x86: fix build with gcc9 Jan Beulich 2019-03-07 10:31 ` [PATCH 1/2] x86/e820: " Jan Beulich 2019-03-07 10:32 ` [PATCH 2/2] x86/mtrr: " Jan Beulich @ 2019-03-07 11:12 ` Wei Liu 2019-03-07 11:37 ` M A Young 2019-03-15 12:33 ` Ping: " Jan Beulich 4 siblings, 0 replies; 22+ messages in thread From: Wei Liu @ 2019-03-07 11:12 UTC (permalink / raw) To: Jan Beulich Cc: Charles Arnold, Juergen Gross, Wei Liu, Andrew Cooper, xen-devel, Roger Pau Monne On Thu, Mar 07, 2019 at 03:23:44AM -0700, Jan Beulich wrote: > Several build issues (new warnings, causing the build to fail due to > -Werror) have been found. The two changes here address the ones > I can actually repro; there are a few more related to > -Waddress-of-packed-member which I haven't been able to see > myself, and hence for now I'm unable to sort out a proper fix / > workaround for them. > > Despite this coming late, I think this is a worthwhile change for 4.12 > with pretty low risk. > > 1: e820: fix build with gcc9 > 2: mtrr: fix build with gcc9 > Reviewed-by: Wei Liu <wei.liu2@citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/2] x86: fix build with gcc9 2019-03-07 10:23 [PATCH 0/2] x86: fix build with gcc9 Jan Beulich ` (2 preceding siblings ...) 2019-03-07 11:12 ` [PATCH 0/2] x86: " Wei Liu @ 2019-03-07 11:37 ` M A Young 2019-03-07 11:57 ` Jan Beulich 2019-03-15 12:33 ` Ping: " Jan Beulich 4 siblings, 1 reply; 22+ messages in thread From: M A Young @ 2019-03-07 11:37 UTC (permalink / raw) To: Jan Beulich Cc: Charles Arnold, Juergen Gross, Wei Liu, Andrew Cooper, xen-devel, Roger Pau Monne On Thu, 7 Mar 2019, Jan Beulich wrote: > Several build issues (new warnings, causing the build to fail due to > -Werror) have been found. The two changes here address the ones > I can actually repro; there are a few more related to > -Waddress-of-packed-member which I haven't been able to see > myself, and hence for now I'm unable to sort out a proper fix / > workaround for them. I found issues with -Waddress-of-packed-member in 3 places in Fedora. My (possibly flawed) attempt to fix the gcc9 issues (on 4.11) is at https://src.fedoraproject.org/rpms/xen/blob/master/f/xen.gcc9.fixes.patch I am running an install of xen with the patch applied so it seems functional. Michael Young _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/2] x86: fix build with gcc9 2019-03-07 11:37 ` M A Young @ 2019-03-07 11:57 ` Jan Beulich 0 siblings, 0 replies; 22+ messages in thread From: Jan Beulich @ 2019-03-07 11:57 UTC (permalink / raw) To: M A Young Cc: Charles Arnold, Juergen Gross, Wei Liu, Andrew Cooper, xen-devel, Roger Pau Monne >>> On 07.03.19 at 12:37, <m.a.young@durham.ac.uk> wrote: > On Thu, 7 Mar 2019, Jan Beulich wrote: > >> Several build issues (new warnings, causing the build to fail due to >> -Werror) have been found. The two changes here address the ones >> I can actually repro; there are a few more related to >> -Waddress-of-packed-member which I haven't been able to see >> myself, and hence for now I'm unable to sort out a proper fix / >> workaround for them. > > I found issues with -Waddress-of-packed-member in 3 places in Fedora. My > (possibly flawed) attempt to fix the gcc9 issues (on 4.11) is at > https://src.fedoraproject.org/rpms/xen/blob/master/f/xen.gcc9.fixes.patch So here we see how effort gets duplicated: What was found here internally was never posted. What you've found was also never posted. Who knows how many more distros carry some for of fix (or perhaps better workaround) for this. May I ask everyone that such fixes be posted on xen-devel, even if the quick-and-dirty solutions may not be what we want to commit? It'll at least allow everyone to know where the issues are and to think of suitable code changes. Also sadly your reply doesn't really help me understand why I'm not myself seeing those -Waddress-of-packed-member issues. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Ping: [PATCH 0/2] x86: fix build with gcc9 2019-03-07 10:23 [PATCH 0/2] x86: fix build with gcc9 Jan Beulich ` (3 preceding siblings ...) 2019-03-07 11:37 ` M A Young @ 2019-03-15 12:33 ` Jan Beulich 4 siblings, 0 replies; 22+ messages in thread From: Jan Beulich @ 2019-03-15 12:33 UTC (permalink / raw) To: Andrew Cooper; +Cc: Charles Arnold, xen-devel, Wei Liu, Roger Pau Monne >>> On 07.03.19 at 11:23, <JBeulich@suse.com> wrote: > Several build issues (new warnings, causing the build to fail due to > -Werror) have been found. The two changes here address the ones > I can actually repro; there are a few more related to > -Waddress-of-packed-member which I haven't been able to see > myself, and hence for now I'm unable to sort out a proper fix / > workaround for them. > > Despite this coming late, I think this is a worthwhile change for 4.12 > with pretty low risk. > > 1: e820: fix build with gcc9 > 2: mtrr: fix build with gcc9 > > Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
[parent not found: <5C80F130020000780021C602@suse.com>]
* Re: [PATCH 0/2] x86: fix build with gcc9 [not found] <5C80F130020000780021C602@suse.com> @ 2019-03-07 10:53 ` Juergen Gross 0 siblings, 0 replies; 22+ messages in thread From: Juergen Gross @ 2019-03-07 10:53 UTC (permalink / raw) To: Jan Beulich, xen-devel Cc: Charles Arnold, Andrew Cooper, Wei Liu, Roger Pau Monne On 07/03/2019 11:23, Jan Beulich wrote: > Several build issues (new warnings, causing the build to fail due to > -Werror) have been found. The two changes here address the ones > I can actually repro; there are a few more related to > -Waddress-of-packed-member which I haven't been able to see > myself, and hence for now I'm unable to sort out a proper fix / > workaround for them. > > Despite this coming late, I think this is a worthwhile change for 4.12 > with pretty low risk. The risk might be low, but taking those patches would result in another round of testing needed. In other words: I'd prefer to defer the patches to 4.13. Juergen _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2019-06-17 16:08 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-07 10:23 [PATCH 0/2] x86: fix build with gcc9 Jan Beulich 2019-03-07 10:31 ` [PATCH 1/2] x86/e820: " Jan Beulich 2019-03-07 10:46 ` Roger Pau Monné 2019-03-07 10:55 ` Wei Liu 2019-03-15 16:07 ` Andrew Cooper 2019-03-18 10:00 ` Jan Beulich 2019-03-07 10:32 ` [PATCH 2/2] x86/mtrr: " Jan Beulich 2019-03-07 10:55 ` Roger Pau Monné 2019-03-07 11:22 ` Jan Beulich 2019-03-07 14:20 ` Roger Pau Monné 2019-03-15 16:21 ` Andrew Cooper 2019-03-18 10:11 ` Jan Beulich 2019-03-18 10:30 ` Andrew Cooper 2019-03-18 10:53 ` Jan Beulich [not found] ` <5C80F32C0200000000103FF7@prv1-mh.provo.novell.com> [not found] ` <5C80F32C0200007800232900@prv1-mh.provo.novell.com> [not found] ` <5C80F32C0200000000104D67@prv1-mh.provo.novell.com> [not found] ` <5C80F32C0200007800238665@prv1-mh.provo.novell.com> 2019-06-14 15:56 ` [Xen-devel] Ping: " Jan Beulich 2019-06-17 15:47 ` Andrew Cooper 2019-06-17 16:08 ` Jan Beulich 2019-03-07 11:12 ` [PATCH 0/2] x86: " Wei Liu 2019-03-07 11:37 ` M A Young 2019-03-07 11:57 ` Jan Beulich 2019-03-15 12:33 ` Ping: " Jan Beulich [not found] <5C80F130020000780021C602@suse.com> 2019-03-07 10:53 ` Juergen Gross
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 NNTP newsgroup(s).