Re: CVS commit: src/lib/libc/stdlib
On Sun, Feb 23, 2020 at 03:35:19AM +0100, Kamil Rytarowski wrote: > Algorithm would be changed from calculating on 32bit numbers with signed > integer overflows to an algorithm calculating on 64bit numbers. The > __dorand48() function truncates the result to least significant 16bits > only so it does not matter. I retained operations on 32bits avoiding > changes of types for stylistic reasons. I am with uwe here - either it would not make any difference at all (on 32bit architectures) or it would end up with the same results and would make no performance difference (on 64 bit architectures), so going with the consistent (unsigned long) would have been fine. Even better would be a cleanup to make it (uint32_t) everwhere, but of course only after carefull examination. Source code consistency is a very important stylistic plus, every break of that should be accompanied by a comment. Martin
Re: CVS commit: src/lib/libc/stdlib
On Sun, Feb 23, 2020 at 03:35:19 +0100, Kamil Rytarowski wrote: > On 23.02.2020 03:20, Valery Ushakov wrote: > > On Sun, Feb 23, 2020 at 02:51:49 +0100, Kamil Rytarowski wrote: > > > >> On 23.02.2020 02:29, Valery Ushakov wrote: > >>> On Sat, Feb 22, 2020 at 14:07:57 +, Kamil Rytarowski wrote: > >>> > Module Name: src > Committed By:kamil > Date:Sat Feb 22 14:07:57 UTC 2020 > > Modified Files: > src/lib/libc/stdlib: _rand48.c > > Log Message: > Avoid undefined behavior in the rand48(3) implementation > > Instead of implicid promotion to signed int, > explicitly cast the arguments to unsigned int. > >>> > >>> Please, please, please, pay at least some attention to what is going > >>> on around the code you are changing. > >>> > >>> If there's already code in this function that does: > >>> > >>>accu = (unsigned long) __rand48_mult[0] * (unsigned long) xseed[0]; > >>> > >>> then keep it consistent and don't do casts to a different type > >>> > >>>accu += (unsigned int) __rand48_mult[0] * (unsigned int) xseed[2]; > >> > >> cast to unsigned long still works, but changes algorithm. My change was > >> performed deliberately. On the other hand and according to local tests > >> the end-result for unsigned long produces the same reults as cast to > >> unsigned int and unsigned char so it does not matter. > > > > I cannot make sense of your answer. Does the cast to unsigned long > > there change the algorithm or does it produce the same result? If it > > produces the same result, then it should be used to be consistent with > > the rest of the code (or the rest of the code changed to use unsigned > > int). If it does change the result, there should be a comment > > explaining it. > > Algorithm would be changed from calculating on 32bit numbers with signed > integer overflows to an algorithm calculating on 64bit numbers. The > __dorand48() function truncates the result to least significant 16bits > only so it does not matter. I retained operations on 32bits avoiding > changes of types for stylistic reasons. That still doesn't make sense to me. You took your time to figure out whats going on in this bit of code. Then you make a change that looks extremely unobvious b/c it is inconsistent with the rest of the code, and you say you did this for stylistic reasons. The next person looking at that code (in $bignum years) will have to waste their time puzzling out the reason. Why not use the knowledge you've gained of this code for good and change the code properly? The 90s unsigned long was probably meant to be 32-bit anyway (cf. X11 mess up with using long for 32 bit quantities in the protocol and then running into issues when 64-bit happened). So if doing things in 32-bit here is the right and intended thing to do, then change that unsigned long to uint32_t. If you don't want to dive that deep (which is entirely understandable), then, exactly for stylistic reasons, cast to unsigned long to be consistent with the old code - as you already established that it didn't change the result. -uwe
Re: CVS commit: src/lib/libc/stdlib
On 23.02.2020 03:20, Valery Ushakov wrote: > On Sun, Feb 23, 2020 at 02:51:49 +0100, Kamil Rytarowski wrote: > >> On 23.02.2020 02:29, Valery Ushakov wrote: >>> On Sat, Feb 22, 2020 at 14:07:57 +, Kamil Rytarowski wrote: >>> Module Name: src Committed By: kamil Date: Sat Feb 22 14:07:57 UTC 2020 Modified Files: src/lib/libc/stdlib: _rand48.c Log Message: Avoid undefined behavior in the rand48(3) implementation Instead of implicid promotion to signed int, explicitly cast the arguments to unsigned int. >>> >>> Please, please, please, pay at least some attention to what is going >>> on around the code you are changing. >>> >>> If there's already code in this function that does: >>> >>>accu = (unsigned long) __rand48_mult[0] * (unsigned long) xseed[0]; >>> >>> then keep it consistent and don't do casts to a different type >>> >>>accu += (unsigned int) __rand48_mult[0] * (unsigned int) xseed[2]; >> >> cast to unsigned long still works, but changes algorithm. My change was >> performed deliberately. On the other hand and according to local tests >> the end-result for unsigned long produces the same reults as cast to >> unsigned int and unsigned char so it does not matter. > > I cannot make sense of your answer. Does the cast to unsigned long > there change the algorithm or does it produce the same result? If it > produces the same result, then it should be used to be consistent with > the rest of the code (or the rest of the code changed to use unsigned > int). If it does change the result, there should be a comment > explaining it. > > -uwe > Algorithm would be changed from calculating on 32bit numbers with signed integer overflows to an algorithm calculating on 64bit numbers. The __dorand48() function truncates the result to least significant 16bits only so it does not matter. I retained operations on 32bits avoiding changes of types for stylistic reasons. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/lib/libc/stdlib
On Sun, Feb 23, 2020 at 02:51:49 +0100, Kamil Rytarowski wrote: > On 23.02.2020 02:29, Valery Ushakov wrote: > > On Sat, Feb 22, 2020 at 14:07:57 +, Kamil Rytarowski wrote: > > > >> Module Name: src > >> Committed By: kamil > >> Date: Sat Feb 22 14:07:57 UTC 2020 > >> > >> Modified Files: > >>src/lib/libc/stdlib: _rand48.c > >> > >> Log Message: > >> Avoid undefined behavior in the rand48(3) implementation > >> > >> Instead of implicid promotion to signed int, > >> explicitly cast the arguments to unsigned int. > > > > Please, please, please, pay at least some attention to what is going > > on around the code you are changing. > > > > If there's already code in this function that does: > > > >accu = (unsigned long) __rand48_mult[0] * (unsigned long) xseed[0]; > > > > then keep it consistent and don't do casts to a different type > > > >accu += (unsigned int) __rand48_mult[0] * (unsigned int) xseed[2]; > > cast to unsigned long still works, but changes algorithm. My change was > performed deliberately. On the other hand and according to local tests > the end-result for unsigned long produces the same reults as cast to > unsigned int and unsigned char so it does not matter. I cannot make sense of your answer. Does the cast to unsigned long there change the algorithm or does it produce the same result? If it produces the same result, then it should be used to be consistent with the rest of the code (or the rest of the code changed to use unsigned int). If it does change the result, there should be a comment explaining it. -uwe
Re: CVS commit: src/lib/libc/stdlib
On 23.02.2020 02:29, Valery Ushakov wrote: > On Sat, Feb 22, 2020 at 14:07:57 +, Kamil Rytarowski wrote: > >> Module Name: src >> Committed By:kamil >> Date:Sat Feb 22 14:07:57 UTC 2020 >> >> Modified Files: >> src/lib/libc/stdlib: _rand48.c >> >> Log Message: >> Avoid undefined behavior in the rand48(3) implementation >> >> Instead of implicid promotion to signed int, >> explicitly cast the arguments to unsigned int. > > Please, please, please, pay at least some attention to what is going > on around the code you are changing. > > If there's already code in this function that does: > >accu = (unsigned long) __rand48_mult[0] * (unsigned long) xseed[0]; > > then keep it consistent and don't do casts to a different type > >accu += (unsigned int) __rand48_mult[0] * (unsigned int) xseed[2]; > > > -uwe > cast to unsigned long still works, but changes algorithm. My change was performed deliberately. On the other hand and according to local tests the end-result for unsigned long produces the same reults as cast to unsigned int and unsigned char so it does not matter. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/lib/libc/stdlib
On Sat, Feb 22, 2020 at 14:07:57 +, Kamil Rytarowski wrote: > Module Name: src > Committed By: kamil > Date: Sat Feb 22 14:07:57 UTC 2020 > > Modified Files: > src/lib/libc/stdlib: _rand48.c > > Log Message: > Avoid undefined behavior in the rand48(3) implementation > > Instead of implicid promotion to signed int, > explicitly cast the arguments to unsigned int. Please, please, please, pay at least some attention to what is going on around the code you are changing. If there's already code in this function that does: accu = (unsigned long) __rand48_mult[0] * (unsigned long) xseed[0]; then keep it consistent and don't do casts to a different type accu += (unsigned int) __rand48_mult[0] * (unsigned int) xseed[2]; -uwe
Re: CVS commit: src/tests/modules
On 22.02.2020 15:54, Paul Goyette wrote: > On Sat, 22 Feb 2020, Kamil Rytarowski wrote: > > While there, it would be good to implement modctl(MODCTL_MODSTAT, > ) to check whether a specific module is loaded into the kernel > and retrieve modstat_t describing it. > > modstat_t m; > strlcpy(_name, "haxm", MAXMODNAME); > if (modctl(MODCTL_MODSTAT, ) == -1) > err(EXIT_FAILURE, "modctl: haxm"); > > I have got use-cases for these checks and I envision their wider usage > in future. We already have 3 use-cases in ATF tests. I can probably do this fairly quickly. But I'll have to look closer at the argument/result passing, especially WRT the module's list of "required" modules. >>> >>> Thinking a bit more, it's probably easiest just to retrieve the entire >>> list of modules with modctl(MODCTL_STAT, ...) and then scan the returned >>> list and compare against ms_name, as is done in modstat(8). >>> >>> Before I invest much time in this, I'd appreciate other opinions on >>> whether a new option is necessary/desirable. >>> >>> >> >> Performance is probably not critical so it sounds fine. >> >> I would like to have at least get_modstat_info() from t_modctl.c in >> libutil. > > Sure that seems reasonable to me. > > Assuming that noone else objects, please feel free to move it. I > think we should also update the test program to use the new libutil > version (rather than duplicating the code). Also update the libutil > man page? > > I guess that the return type of get_modstat_info() should be changed > to int rather than bool? And that it shouldn't directly print error > messages? :) > > I will do it and share a patch. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/tests/modules
On Sat, 22 Feb 2020, Kamil Rytarowski wrote: While there, it would be good to implement modctl(MODCTL_MODSTAT, ) to check whether a specific module is loaded into the kernel and retrieve modstat_t describing it. modstat_t m; strlcpy(_name, "haxm", MAXMODNAME); if (modctl(MODCTL_MODSTAT, ) == -1) err(EXIT_FAILURE, "modctl: haxm"); I have got use-cases for these checks and I envision their wider usage in future. We already have 3 use-cases in ATF tests. I can probably do this fairly quickly.?? But I'll have to look closer at the argument/result passing, especially WRT the module's list of "required" modules. Thinking a bit more, it's probably easiest just to retrieve the entire list of modules with modctl(MODCTL_STAT, ...) and then scan the returned list and compare against ms_name, as is done in modstat(8). Before I invest much time in this, I'd appreciate other opinions on whether a new option is necessary/desirable. Performance is probably not critical so it sounds fine. I would like to have at least get_modstat_info() from t_modctl.c in libutil. Sure that seems reasonable to me. Assuming that noone else objects, please feel free to move it. I think we should also update the test program to use the new libutil version (rather than duplicating the code). Also update the libutil man page? I guess that the return type of get_modstat_info() should be changed to int rather than bool? And that it shouldn't directly print error messages? :) ++--+---+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired) | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com | | Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org | ++--+---+
Re: CVS commit: src/tests/modules
On 22.02.2020 15:32, Paul Goyette wrote: > On Sat, 22 Feb 2020, Paul Goyette wrote: > >>> While there, it would be good to implement modctl(MODCTL_MODSTAT, >>> ) to check whether a specific module is loaded into the kernel >>> and retrieve modstat_t describing it. >>> >>> modstat_t m; >>> strlcpy(_name, "haxm", MAXMODNAME); >>> if (modctl(MODCTL_MODSTAT, ) == -1) >>> err(EXIT_FAILURE, "modctl: haxm"); >>> >>> I have got use-cases for these checks and I envision their wider usage >>> in future. We already have 3 use-cases in ATF tests. >> >> I can probably do this fairly quickly. But I'll have to look closer >> at the argument/result passing, especially WRT the module's list of >> "required" modules. > > Thinking a bit more, it's probably easiest just to retrieve the entire > list of modules with modctl(MODCTL_STAT, ...) and then scan the returned > list and compare against ms_name, as is done in modstat(8). > > Before I invest much time in this, I'd appreciate other opinions on > whether a new option is necessary/desirable. > > Performance is probably not critical so it sounds fine. I would like to have at least get_modstat_info() from t_modctl.c in libutil. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/tests/modules
On Sat, 22 Feb 2020, Paul Goyette wrote: While there, it would be good to implement modctl(MODCTL_MODSTAT, ) to check whether a specific module is loaded into the kernel and retrieve modstat_t describing it. modstat_t m; strlcpy(_name, "haxm", MAXMODNAME); if (modctl(MODCTL_MODSTAT, ) == -1) err(EXIT_FAILURE, "modctl: haxm"); I have got use-cases for these checks and I envision their wider usage in future. We already have 3 use-cases in ATF tests. I can probably do this fairly quickly. But I'll have to look closer at the argument/result passing, especially WRT the module's list of "required" modules. Thinking a bit more, it's probably easiest just to retrieve the entire list of modules with modctl(MODCTL_STAT, ...) and then scan the returned list and compare against ms_name, as is done in modstat(8). Before I invest much time in this, I'd appreciate other opinions on whether a new option is necessary/desirable. ++--+---+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired) | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com | | Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org | ++--+---+
Re: CVS commit: src/tests/modules
On Sat, 22 Feb 2020, Kamil Rytarowski wrote: I have got no opinion. Please rearrange the directories as needed. It's too much bother for now to move things around. But for future changes it would be good to put new "helper" modules in the same area as the tests being helped. While there, it would be good to implement modctl(MODCTL_MODSTAT, ) to check whether a specific module is loaded into the kernel and retrieve modstat_t describing it. modstat_t m; strlcpy(_name, "haxm", MAXMODNAME); if (modctl(MODCTL_MODSTAT, ) == -1) err(EXIT_FAILURE, "modctl: haxm"); I have got use-cases for these checks and I envision their wider usage in future. We already have 3 use-cases in ATF tests. I can probably do this fairly quickly. But I'll have to look closer at the argument/result passing, especially WRT the module's list of "required" modules. ++--+---+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired) | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com | | Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org | ++--+---+
Re: CVS commit: src/tests/modules
I have got no opinion. Please rearrange the directories as needed. While there, it would be good to implement modctl(MODCTL_MODSTAT, ) to check whether a specific module is loaded into the kernel and retrieve modstat_t describing it. modstat_t m; strlcpy(_name, "haxm", MAXMODNAME); if (modctl(MODCTL_MODSTAT, ) == -1) err(EXIT_FAILURE, "modctl: haxm"); I have got use-cases for these checks and I envision their wider usage in future. We already have 3 use-cases in ATF tests. On 22.02.2020 04:41, Paul Goyette wrote: > OK, I over-reacted and didn't completely read the original commit > message. > > The t_builtin.c stuff is indeed a test-of-module-functionality > so it does belong here. > > But some of the other stuff here does not belong, such as the > threadpool, fetchstore, and kcov stuff. As far as I can see, > those all belong somewhere else, probably in the tests/sys/... > hierarchy. > > Anyway, my apologies for over-reacting to this commit. And > thanks to riastradh@ for pointing this out (on IRC). > > > > On Fri, 21 Feb 2020, Paul Goyette wrote: > >> Really, the tests/modules directory should be only used for tests-that- >> relate-to-module-functionality. It should NOT be used for modules- >> that-support-tests-of-other-functionality. >> >> In the future, please do not put support modules here; put them in the >> samae place as the tests that they support. >> >> >> On Sat, 22 Feb 2020, Kamil Rytarowski wrote: >> >>> Module Name: src >>> Committed By: kamil >>> Date: Sat Feb 22 00:18:55 UTC 2020 >>> >>> Modified Files: >>> src/tests/modules: t_builtin.c >>> >>> Log Message: >>> Avoid undefined behavior in disabledstat >>> >>> t_builtin.c:174:16, member access within misaligned address >>> 0x741271c25004 >>> for type 'struct modstat_t' >>> >>> t_builtin.c:175:4, member access within misaligned address >>> 0x741271c251c4 >>> for type 'struct modstat_t' >>> >>> >>> To generate a diff of this commit: >>> cvs rdiff -u -r1.4 -r1.5 src/tests/modules/t_builtin.c >>> >>> Please note that diffs are not public domain; they are subject to the >>> copyright notices on the relevant files. >>> >>> >>> !DSPAM:5e5073af66043393299806! >>> >>> >> >> ++--+---+ >> | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | >> | (Retired) | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com | >> | Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org | >> ++--+---+ >> > > ++--+---+ > | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | > | (Retired) | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com | > | Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org | > ++--+---+ signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/sys/arch/arm/arm32
On 21/02/2020 23:27, Maya Rashish wrote:
[...]
Index: src/sys/arch/arm/arm32/bus_dma.c
diff -u src/sys/arch/arm/arm32/bus_dma.c:1.118
src/sys/arch/arm/arm32/bus_dma.c:1.119
--- src/sys/arch/arm/arm32/bus_dma.c:1.118 Tue Nov 5 10:21:31 2019
+++ src/sys/arch/arm/arm32/bus_dma.cFri Feb 21 23:27:06 2020
[...]
@@ -404,7 +404,7 @@ _bus_dmamap_create(bus_dma_tag_t t, bus_
#ifdef DEBUG_DMA
printf("dmamap_create:map=%p\n", map);
#endif/* DEBUG_DMA */
- return 0;
+ return error;
}
/*
This isn't correct for the case where _ARM32_NEED_BUS_DMA_BOUNCE isn't
defined.
I'll fix it.
Nick
