Re: CVS commit: src/sys/sys
On Sun, 8 Jul 2018, Christos Zoulas wrote: On Jul 9, 5:56am, p...@whooppee.com (Paul Goyette) wrote: -- Subject: Re: CVS commit: src/sys/sys | > Yes, the double evaluation is a show-stopper, please revert. | | A quick inspection shows that no additional code is generated, at least | when using gcc. For example: Not if the value has side effects: int *flags; CLR(*flags++, 1); Ah, yeah, that would be bad! Thanks for the example. +--+--++ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee dot com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd dot org | +--+--++
Re: CVS commit: src/sys/sys
On Jul 9, 5:56am, p...@whooppee.com (Paul Goyette) wrote: -- Subject: Re: CVS commit: src/sys/sys | > Yes, the double evaluation is a show-stopper, please revert. | | A quick inspection shows that no additional code is generated, at least | when using gcc. For example: Not if the value has side effects: int *flags; CLR(*flags++, 1); christos
Re: CVS commit: src/lib/libpuffs
On Jul 8, 10:12pm, n...@gmx.com (Kamil Rytarowski) wrote: -- Subject: Re: CVS commit: src/lib/libpuffs | How about this patch: | | http://netbsd.org/~kamil/patch-00065-mman-MAP_ALIGNED-cast.txt | | With an optional additional () around the macro body. Yes, that's better! since the flags is expected to be an int. And add the additional parens around the cast, and revert my cast :-) Thanks, christos
Re: CVS commit: src/sys/sys
On Sun, 8 Jul 2018, Christos Zoulas wrote: Committed By: pgoyette Date: Sun Jul 8 06:21:42 UTC 2018 Modified Files: src/sys/sys: types.h Log Message: Use a different, type-insensitive idiom for CLR(). As discussed on IRC and proposed by dholland@, the existing idiom is type-sensitive, and will likely fail silently when the flags variable is a 64-bit type. No functional change intended. If anything breaks, it was probably already broken. This is much worse, since it now will evaluate the expression twice. Please do not just change macros like this without a proper discussion on the mailing lists. Yes, the double evaluation is a show-stopper, please revert. A quick inspection shows that no additional code is generated, at least when using gcc. For example: kern/vfs_bio.c: #1037: cv_signal(_cv); 0x80a3ce88 <+127>: callq 0x80998ef5 #1040: if (ISSET(bp->b_cflags, BC_WANTED)) 0x80a3ce8d <+132>: mov0xec(%rbx),%eax 0x80a3ce93 <+138>: test $0x80,%eax 0x80a3ce98 <+143>: je 0x80a3cea5 #1041: CLR(bp->b_cflags, BC_WANTED|BC_AGE); 0x80a3ce9a <+145>: and$0xff7e,%eax 0x80a3ce9f <+150>: mov%eax,0xec(%rbx) The compiler seems perfectly happy optimizing this code without generating any additional evaluations. HOWEVER, I'll be happy to revert this. Since it was dholland@ who proposed this (on IRC), I will let him initiate any needed discussion on the tech-kern list. +--+--++ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee dot com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd dot org | +--+--++
Re: CVS commit: src/lib/libpuffs
On 08.07.2018 18:48, Christos Zoulas wrote: > Module Name: src > Committed By: christos > Date: Sun Jul 8 16:48:47 UTC 2018 > > Modified Files: > src/lib/libpuffs: callcontext.c > > Log Message: > correct previous cast. > How about this patch: http://netbsd.org/~kamil/patch-00065-mman-MAP_ALIGNED-cast.txt With an optional additional () around the macro body. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/sys/arch/x86/x86
On 08.07.2018 15:56, Christos Zoulas wrote: > In article <20180708092413.gb8...@mail.duskware.de>, > Martin Husemann wrote: >> On Sun, Jul 08, 2018 at 10:49:53AM +0200, Jaromír Dole?ek wrote: Module Name:src Committed By: kamil Date: Sat Jul 7 23:05:50 UTC 2018 Modified Files: src/sys/arch/x86/x86: mpbios.c Log Message: Remove unaligned access to mpbios_page[] Replace unaligned pointer dereference with a more portable construct that is free from Undefined Behavior semantics. sys/arch/x86/x86/mpbios.c:308:11, load of misaligned address 0x800031c7a413 for type 'const >> __uint16_t' which requires 2 byte alignment >> >> >> Can we please do NOT do such stupid changes? >> >> This is a bogus error message, please restore the original code! > > These changes are pointless; how much code will we need to change > to silence mis-aligned warnings? These changes are also dangerous > when it comes to reading from devices (where multiple reads can > behave differently). If you want to silence the warnings, use > __attribute__, but even that is of questionable use. I'd venture > to say, misaligned warnings on cpus where aligned access (to do > the aligning) is costlier than direct misaligned accesses (like > x86) are useless. In addition, it is not like we would ever turn > on the force alignment bit on an x86 cpu and have things work! > > So my preference would be to revert the change and take this up > to tech-kern first (how misaligned accesses should be treated), > because the situation is not that simple (when it comes to things > like SSE operations etc.) > http://pzemtsov.github.io/2016/11/06/bug-story-alignment-on-x86.html > > > Best, > > christos > I've started a tech-kern discussion on the request. Regarding the number of assignment issues to address: - acpica - mpbios.c [this one] - arch/x86/x86/patch.c [load of address with insufficient space] - sys/kern/subr_disk_mbr.c disklabel struct misaligned - IPv6, TCP - amd64/amd64/kobj_machdep.c (ELF) - XHCI http://netbsd.org/~kamil/kubsan/0007-boot-real-hardware.txt I'm going to revert it for a while, until there will be a conclusion. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/sys/sys
In article <20180708092258.ga13...@britannica.bec.de>, Joerg Sonnenberger wrote: >On Sun, Jul 08, 2018 at 06:21:42AM +, Paul Goyette wrote: >> Module Name: src >> Committed By:pgoyette >> Date:Sun Jul 8 06:21:42 UTC 2018 >> >> Modified Files: >> src/sys/sys: types.h >> >> Log Message: >> Use a different, type-insensitive idiom for CLR(). >> >> As discussed on IRC and proposed by dholland@, the existing idiom is >> type-sensitive, and will likely fail silently when the flags variable >> is a 64-bit type. >> >> No functional change intended. If anything breaks, it was probably >> already broken. > >This is much worse, since it now will evaluate the expression twice. >Please do not just change macros like this without a proper discussion >on the mailing lists. Yes, the double evaluation is a show-stopper, please revert. christos
Re: CVS commit: src/sys/arch/x86/x86
On 08.07.2018 10:49, Jaromír Doleček wrote: > Shouldn't this: > > memtop |= (uint16_t)mpbios_page[0x414] << 8; > > be actually << 16 to keep the same semantics? > No. This is a 2-byte (x86 word) variable. One byte has to be stored with the 8 bits shift. If it would be differently it would probably brick the booting process. > Jaromir > Le dim. 8 juil. 2018 à 01:05, Kamil Rytarowski a écrit : >> >> Module Name:src >> Committed By: kamil >> Date: Sat Jul 7 23:05:50 UTC 2018 >> >> Modified Files: >> src/sys/arch/x86/x86: mpbios.c >> >> Log Message: >> Remove unaligned access to mpbios_page[] >> >> Replace unaligned pointer dereference with a more portable construct that >> is free from Undefined Behavior semantics. >> >> sys/arch/x86/x86/mpbios.c:308:11, load of misaligned address >> 0x800031c7a413 for type 'const __uint16_t' which requires 2 byte >> alignment >> >> Detected with Kernel Undefined Behavior Sanitizer >> >> >> To generate a diff of this commit: >> cvs rdiff -u -r1.66 -r1.67 src/sys/arch/x86/x86/mpbios.c >> >> Please note that diffs are not public domain; they are subject to the >> copyright notices on the relevant files. >> signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/sys/arch/x86/x86
In article <20180708092413.gb8...@mail.duskware.de>, Martin Husemann wrote: >On Sun, Jul 08, 2018 at 10:49:53AM +0200, Jaromír Dole?ek wrote: >> > Module Name:src >> > Committed By: kamil >> > Date: Sat Jul 7 23:05:50 UTC 2018 >> > >> > Modified Files: >> > src/sys/arch/x86/x86: mpbios.c >> > >> > Log Message: >> > Remove unaligned access to mpbios_page[] >> > >> > Replace unaligned pointer dereference with a more portable construct that >> > is free from Undefined Behavior semantics. >> > >> > sys/arch/x86/x86/mpbios.c:308:11, load of misaligned address >> > 0x800031c7a413 for type 'const >__uint16_t' which requires 2 byte alignment > > >Can we please do NOT do such stupid changes? > >This is a bogus error message, please restore the original code! These changes are pointless; how much code will we need to change to silence mis-aligned warnings? These changes are also dangerous when it comes to reading from devices (where multiple reads can behave differently). If you want to silence the warnings, use __attribute__, but even that is of questionable use. I'd venture to say, misaligned warnings on cpus where aligned access (to do the aligning) is costlier than direct misaligned accesses (like x86) are useless. In addition, it is not like we would ever turn on the force alignment bit on an x86 cpu and have things work! So my preference would be to revert the change and take this up to tech-kern first (how misaligned accesses should be treated), because the situation is not that simple (when it comes to things like SSE operations etc.) http://pzemtsov.github.io/2016/11/06/bug-story-alignment-on-x86.html Best, christos
Re: CVS commit: src/sys/sys
On Sun, Jul 08, 2018 at 06:21:42AM +, Paul Goyette wrote: > Module Name: src > Committed By: pgoyette > Date: Sun Jul 8 06:21:42 UTC 2018 > > Modified Files: > src/sys/sys: types.h > > Log Message: > Use a different, type-insensitive idiom for CLR(). > > As discussed on IRC and proposed by dholland@, the existing idiom is > type-sensitive, and will likely fail silently when the flags variable > is a 64-bit type. > > No functional change intended. If anything breaks, it was probably > already broken. This is much worse, since it now will evaluate the expression twice. Please do not just change macros like this without a proper discussion on the mailing lists. Joerg
Re: CVS commit: src/sys/arch/x86/x86
On Sun, Jul 08, 2018 at 10:49:53AM +0200, Jaromír Dole?ek wrote: > > Module Name:src > > Committed By: kamil > > Date: Sat Jul 7 23:05:50 UTC 2018 > > > > Modified Files: > > src/sys/arch/x86/x86: mpbios.c > > > > Log Message: > > Remove unaligned access to mpbios_page[] > > > > Replace unaligned pointer dereference with a more portable construct that > > is free from Undefined Behavior semantics. > > > > sys/arch/x86/x86/mpbios.c:308:11, load of misaligned address > > 0x800031c7a413 for type 'const __uint16_t' which requires 2 byte > > alignment Can we please do NOT do such stupid changes? This is a bogus error message, please restore the original code! Martin
Re: CVS commit: src/sys/arch/x86/x86
Shouldn't this: memtop |= (uint16_t)mpbios_page[0x414] << 8; be actually << 16 to keep the same semantics? Jaromir Le dim. 8 juil. 2018 à 01:05, Kamil Rytarowski a écrit : > > Module Name:src > Committed By: kamil > Date: Sat Jul 7 23:05:50 UTC 2018 > > Modified Files: > src/sys/arch/x86/x86: mpbios.c > > Log Message: > Remove unaligned access to mpbios_page[] > > Replace unaligned pointer dereference with a more portable construct that > is free from Undefined Behavior semantics. > > sys/arch/x86/x86/mpbios.c:308:11, load of misaligned address > 0x800031c7a413 for type 'const __uint16_t' which requires 2 byte alignment > > Detected with Kernel Undefined Behavior Sanitizer > > > To generate a diff of this commit: > cvs rdiff -u -r1.66 -r1.67 src/sys/arch/x86/x86/mpbios.c > > Please note that diffs are not public domain; they are subject to the > copyright notices on the relevant files. >
