Re: CVS commit: src/sys/compat/sys
> On Jun 28, 2020, at 10:24 AM, Robert Elz wrote: > >Date:Sat, 27 Jun 2020 11:49:30 -0400 >From:"Christos Zoulas" >Message-ID: <20200627154930.84e22f...@cvs.netbsd.org> > > | Modified Files: > |src/sys/compat/sys: mount.h > | > | Log Message: > | Ignore the supplied size, and always use the argument size that we know. > > Is this fix correct?Certainly looks more reasonable than > what was there before, as the supplied size (for no seemingly > good reason) is often 0, but in compat_20_sys_getfsstat() (in > sys/compat/common/vfs_syscalls_20.c) statvfs_to_statfs12_copy() > is called, via do_sys_getvfsstat() with a size of > sizeof(struct statvfs90) and a statvfs90 (compat/sys/statvfs.h) > is certainly not the same size as a statfs12 (compat/sys/mount.h) > > Or perhaps (probably more likely) compat_20_sys_getfsstat() should > be passing sizeof(struct statfs12) instead ? (And now may as well > just pass 0). No, it has to be sizeof(struct statfs12) because the function fills an array of struct statfs12 and needs to know how much to increment. Thanks, christos signature.asc Description: Message signed with OpenPGP
Re: CVS commit: src/sys/compat/sys
Date:Sat, 27 Jun 2020 11:49:30 -0400 From:"Christos Zoulas" Message-ID: <20200627154930.84e22f...@cvs.netbsd.org> | Modified Files: | src/sys/compat/sys: mount.h | | Log Message: | Ignore the supplied size, and always use the argument size that we know. Is this fix correct?Certainly looks more reasonable than what was there before, as the supplied size (for no seemingly good reason) is often 0, but in compat_20_sys_getfsstat() (in sys/compat/common/vfs_syscalls_20.c) statvfs_to_statfs12_copy() is called, via do_sys_getvfsstat() with a size of sizeof(struct statvfs90) and a statvfs90 (compat/sys/statvfs.h) is certainly not the same size as a statfs12 (compat/sys/mount.h) Or perhaps (probably more likely) compat_20_sys_getfsstat() should be passing sizeof(struct statfs12) instead ? (And now may as well just pass 0). kre
Re: CVS commit: src/sys/compat/sys
On Fri, Jun 28, 2019 at 05:03:37AM -0700, Jason Thorpe wrote: > > > On Jun 26, 2019, at 7:10 PM, matthew green wrote: > > > >> Always include the 32 bit structure and definitions on _LP64 regardless > >> of compat32 being on or off, because we want the headers to work when > >> compiling modular kernels. Of course the 32 bit structs do not make sense > >> on platforms that don't have 32 bit modes (alpha), but we don't have > >> a define for that and it does not hurt. > > > > i've been using _LP64 && !__alpha__ for this when it strikes. > > > > sub-optimal, but also easy to grep and find :-) > > Perhaps we should define "_LP64_ONLY" in for this type of > situation? > I'm a really huge fan of keeping structs the same across archs when it doesn't cost us very much. It's been a real blessing that netbsd is mostly consistent when porting programming languages, which often end up embedding a list of structs and their sizes, generated by very fragile code (or by hand!)
Re: CVS commit: src/sys/compat/sys
> On Jun 26, 2019, at 7:10 PM, matthew green wrote: > >> Always include the 32 bit structure and definitions on _LP64 regardless >> of compat32 being on or off, because we want the headers to work when >> compiling modular kernels. Of course the 32 bit structs do not make sense >> on platforms that don't have 32 bit modes (alpha), but we don't have >> a define for that and it does not hurt. > > i've been using _LP64 && !__alpha__ for this when it strikes. > > sub-optimal, but also easy to grep and find :-) Perhaps we should define "_LP64_ONLY" in for this type of situation? -- thorpej
re: CVS commit: src/sys/compat/sys
> Always include the 32 bit structure and definitions on _LP64 regardless > of compat32 being on or off, because we want the headers to work when > compiling modular kernels. Of course the 32 bit structs do not make sense > on platforms that don't have 32 bit modes (alpha), but we don't have > a define for that and it does not hurt. i've been using _LP64 && !__alpha__ for this when it strikes. sub-optimal, but also easy to grep and find :-) .mrg.
Re: CVS commit: src/sys/compat/sys
> Date: Fri, 15 Jun 2018 19:55:28 +0700
> From: Robert Elz
>
> Another way would be
>
> static const struct timespec50 zts = { 0 };
> and
> *ts50 = zts;
I have nothing substantive to add about the question at hand, but one
tiny nit: there is no need for the initializer in this case, and
indeed if you have -Werror=missing-field-initializers as I think we do
by default, writing out the full initializer is a pain.
It suffices to say
static const struct timespec50 zts;
and the implementation will guarantee it is initialized to all
zero/null as appropriate.
Re: CVS commit: src/sys/compat/sys
On Fri, Jun 15, 2018 at 17:11:49 +0300, Valery Ushakov wrote: > It seems that with restrict we can just memset the whole struct and > the compiler will elide the memset completely when there's no pad to > scrub. Alas, with the real code the compiler cannot elide the memset. :( -uwe
Re: CVS commit: src/sys/compat/sys
On Fri, Jun 15, 2018 at 19:55:28 +0700, Robert Elz wrote:
> Date:Fri, 15 Jun 2018 12:41:56 +0300
> From:Valery Ushakov
> Message-ID: <20180615094156.gd3...@pony.stderr.spb.ru>
>
> | Re memset - I now wonder if
> | compiler is even allowed to be smart here b/c strict aliasing
>
> Another way would be
>
> static const struct timespec50 zts = { 0 };
> and
> *ts50 = zts;
>
> which is unlikely to have that problem, and which the compiler might
> be smart enough to optimise into a single assignment where there is
> padding, and perhaps even nothing where there is none.
The compiler does field-by-field assignment in that case and uses
"narrow" movl that doesn't scrub the pad.
With memset you seem to also need to add "restrict" so that the
compiler can elide the dead moves.
The example I'm playing with is:
#include
struct s { int i; long l; };
struct s64 { long i; long l; };
//#define restrict
void foo(struct s * restrict l, const struct s64 * restrict r)
{
#if 0
static const struct s s0 = { 0, 0 };
*l = s0;
#else
memset(l, 0, sizeof(*l));
#endif
l->i = r->i;
l->l = r->l;
}
> I'm also not really a fan of the #if that tests the number of bits in
> int and long (by testing their max values) - that is assuming that
> when long is wider than int, it also has wider alignment, which is
> not necessarily so - a system with 4 byte ints and 8 byte long,
> could have long with 4 byte alignment (and that struct be 12 bytes
> long, alogned 4). The code as it is isn't wrong, ijust the attempt
> to avoid the memset() when it isn't needed might fail, and do the
> memset() when there is nothing to clear - and if the compiler doesn't
> or even can't, optimise it away then it is wasteful.
Yes, the #if test feels icky...
It seems that with restrict we can just memset the whole struct and
the compiler will elide the memset completely when there's no pad to
scrub.
-uwe
Re: CVS commit: src/sys/compat/sys
Date:Fri, 15 Jun 2018 12:41:56 +0300
From:Valery Ushakov
Message-ID: <20180615094156.gd3...@pony.stderr.spb.ru>
| Re memset - I now wonder if
| compiler is even allowed to be smart here b/c strict aliasing
Another way would be
static const struct timespec50 zts = { 0 };
and
*ts50 = zts;
which is unlikely to have that problem, and which the compiler might
be smart enough to optimise into a single assignment where there is
padding, and perhaps even nothing where there is none.
I'm also not really a fan of the #if that tests the number of bits in
int and long (by testing their max values) - that is assuming that
when long is wider than int, it also has wider alignment, which is
not necessarily so - a system with 4 byte ints and 8 byte long,
could have long with 4 byte alignment (and that struct be 12 bytes
long, alogned 4). The code as it is isn't wrong, ijust the attempt
to avoid the memset() when it isn't needed might fail, and do the
memset() when there is nothing to clear - and if the compiler doesn't
or even can't, optimise it away then it is wasteful.
kre
Re: CVS commit: src/sys/compat/sys
On Fri, Jun 15, 2018 at 18:05:29 +1000, matthew green wrote: > "Robert Elz" writes: > > Module Name:src > > Committed By: kre > > Date: Fri Jun 15 07:46:59 UTC 2018 > > > > Modified Files: > > src/sys/compat/sys: time_types.h > > > > Log Message: > > If we are going to use offsetof() we'd need to include to > > get it defined. Rather than deal with potential namespace issues > > with that, just clear the entire struct, rather than attempting to > > stop after the potential padding field. If the compiler is good enough > > it should make no difference (there are just 3 fields, 2 named ones > > are assigned to, immediately after the memset() - the compiler can > > detect that, and not bother assigning (via memset()) to the unmamed > > 3rd padding field). If the compiler is not smart enough to deal > > with this, then I doubt writing 8 more zero bytes will make enough > > difference to matter. > > the compiler isn't smart apparently, and the previous change > should have fixed the problem. did we really need this too? Oh, thanks for fixing it, my bad. Re memset - I now wonder if compiler is even allowed to be smart here b/c strict aliasing (but I'm not re^n-reading that part of the standard just to satisfy my idle curiosity :) -uwe
re: CVS commit: src/sys/compat/sys
"Robert Elz" writes: > Module Name: src > Committed By: kre > Date: Fri Jun 15 07:46:59 UTC 2018 > > Modified Files: > src/sys/compat/sys: time_types.h > > Log Message: > If we are going to use offsetof() we'd need to include to > get it defined. Rather than deal with potential namespace issues > with that, just clear the entire struct, rather than attempting to > stop after the potential padding field. If the compiler is good enough > it should make no difference (there are just 3 fields, 2 named ones > are assigned to, immediately after the memset() - the compiler can > detect that, and not bother assigning (via memset()) to the unmamed > 3rd padding field). If the compiler is not smart enough to deal > with this, then I doubt writing 8 more zero bytes will make enough > difference to matter. the compiler isn't smart apparently, and the previous change should have fixed the problem. did we really need this too? .mrg.
Re: CVS commit: src/sys/compat/sys
On Tue, 20 Dec 2011, Matthias Drochner wrote: Module Name:src Committed By: drochner Date: Tue Dec 20 16:38:06 UTC 2011 Modified Files: src/sys/compat/sys: rnd.h Log Message: allow kernels w/o COMPAT_50 to build What was the actual problem? Nothing defined by this file is supposed to be used in a kernel without COMPAT_50; if something is being used accidentally then I'd like to fix that. --apb (Alan Barrett)
Re: CVS commit: src/sys/compat/sys
On Tue, 20 Dec 2011, Alan Barrett wrote: On Tue, 20 Dec 2011, Matthias Drochner wrote: Modified Files: src/sys/compat/sys: rnd.h Log Message: allow kernels w/o COMPAT_50 to build What was the actual problem? Nothing defined by this file is supposed to be used in a kernel without COMPAT_50; if something is being used accidentally then I'd like to fix that. OK, I found it. rndpseudo_50.o is unconditionally compiled and added to libcompat in the kernel build directory. Everything else in sys/compat/common is handled in the same way. I am inclined to wrap most of the contents of compat/common/rndpseudo_50.c and compat/sys/rnd.h in #ifdef COMPAT_50 guards, although other files in compat/common and compat/sys do not seem to do this. --apb (Alan Barrett)
