Re: CVS commit: src/share/man/man7
On Sat, Nov 25, 2023 at 11:36:34 -0800, Alistair Crooks wrote: > I find it better to have > > MAN+= bar.7 > MAN+= foo.7 > > Since a grep for 'MAN.*foo' will produce meaningful results Also good for diffs in the future, e.g. MAN += f.3 -MAN += g.3 MAN += h.3 vs. f.3 \ -g.3 \ h.3 \ Diff -p *might* do the helpful thing, but it gets confused sometimes. -uwe
Re: CVS commit: src/share/man/man7
On Fri, Nov 24, 2023 at 18:09 Taylor R Campbell < campbell+netbsd-source-change...@mumble.net> wrote: > > Date: Sat, 25 Nov 2023 02:05:25 + > > From: Taylor R Campbell > > > > > Date: Sat, 25 Nov 2023 00:23:33 - (UTC) > > > From: chris...@astron.com (Christos Zoulas) > > > > > > Yes, this is indeed a lot better. I prefer though: > > > > > > MAN+= \ > > > bar.7 \ > > > foo.7 > > > > > > It is faster to parse, involves less typing, whitespace is cleaner. > > > > This one doesn't have the same pattern for every line, so it makes > > merging and sorting harder -- do M-x sort-lines on the content lines, > > and you'll come up with: > > > > MAN+= \ > > foo.7 > > bar.7 \ > > err, obviously I meant this example the other way; if it had been > written as: > > MAN+= \ > foo.7 \ > bar.7 > > as the natural order of metasyntactic variables (foo, bar), then doing > M-x sort-lines on the content lines would yield: > > MAN+= \ > bar.7 > foo.7 \ I find it better to have MAN+= bar.7 MAN+= foo.7 Since a grep for 'MAN.*foo' will produce meaningful results Sorting, and initial entry are once-only operations, searching happens everywhere all the time. > > >
Re: CVS commit: src/share/man/man7
> Date: Sat, 25 Nov 2023 02:05:25 + > From: Taylor R Campbell > > > Date: Sat, 25 Nov 2023 00:23:33 - (UTC) > > From: chris...@astron.com (Christos Zoulas) > > > > Yes, this is indeed a lot better. I prefer though: > > > > MAN+= \ > > bar.7 \ > > foo.7 > > > > It is faster to parse, involves less typing, whitespace is cleaner. > > This one doesn't have the same pattern for every line, so it makes > merging and sorting harder -- do M-x sort-lines on the content lines, > and you'll come up with: > > MAN+= \ > foo.7 > bar.7 \ err, obviously I meant this example the other way; if it had been written as: MAN+= \ foo.7 \ bar.7 as the natural order of metasyntactic variables (foo, bar), then doing M-x sort-lines on the content lines would yield: MAN+= \ bar.7 foo.7 \
Re: CVS commit: src/share/man/man7
> Date: Sat, 25 Nov 2023 00:23:33 - (UTC) > From: chris...@astron.com (Christos Zoulas) > > Yes, this is indeed a lot better. I prefer though: > > MAN+= \ > bar.7 \ > foo.7 > > It is faster to parse, involves less typing, whitespace is cleaner. This one doesn't have the same pattern for every line, so it makes merging and sorting harder -- do M-x sort-lines on the content lines, and you'll come up with: MAN+= \ foo.7 bar.7 \ which does the wrong thing, and only if you're lucky will fail in an obvious way rather than just silently omitting some entries. In portable makefiles I usually write: MAN= \ bar.7 \ foo.7 \ # end of MAN so at least each line has the same pattern, making merging and sorting easier.
Re: CVS commit: src/share/man/man7
In article <20231123211614.011a1f...@cvs.netbsd.org>, Taylor R Campbell wrote: >-=-=-=-=-=- > >Module Name: src >Committed By: riastradh >Date: Thu Nov 23 21:16:13 UTC 2023 > >Modified Files: > src/share/man/man7: Makefile > >Log Message: >share/man/man7/Makefile: Split MAN on separate lines, and sort. > >Makes sorting and merging changes easier. > >No functional change intended. > >Preparing for a new stack(7) in the service of PR pkg/57708. Yes, this is indeed a lot better. I prefer though: MAN+= \ bar.7 \ foo.7 It is faster to parse, involves less typing, whitespace is cleaner. christos
Re: CVS commit: src/lib/libc/ssp
On Wednesday, November 15, 2023 4:15:28 AM CET you wrote: > Module Name: src > Committed By: christos > Date: Wed Nov 15 03:15:28 UTC 2023 > > Modified Files: > src/lib/libc/ssp: Makefile.inc > Added Files: > src/lib/libc/ssp: ssp_redirect.c > > Log Message: > provide materialized functions for the ssp overriden inlines The functions are supposed to be transparent and they used to be. Can we please just go back to the working state before? IMO wanting to overriding getcwd is absolutely no justification for this. If the prototype (and inline function) is visible from the header, userland should *not* be abled to interpose it. If it is not visible due to standard headers, there was no problem in first place. Joerg
Re: CVS commit: src/sys/arch/mips/mips
Thank you, I should pay attention to that. On Wed, Oct 25, 2023 at 9:02 AM Nick Hudson wrote: > > Module Name:src > Committed By: skrll > Date: Wed Oct 25 06:02:14 UTC 2023 > > Modified Files: > src/sys/arch/mips/mips: kgdb_machdep.c > > Log Message: > -> > > > To generate a diff of this commit: > cvs rdiff -u -r1.21 -r1.22 src/sys/arch/mips/mips/kgdb_machdep.c > > Please note that diffs are not public domain; they are subject to the > copyright notices on the relevant files. >
Re: CVS commit: src/tests/sbin/ifconfig
On 2023/10/18 17:25, Rin Okuyama wrote: Module Name:src Committed By: rin Date: Wed Oct 18 08:25:14 UTC 2023 Modified Files: src/tests/sbin/ifconfig: t_capabilities.sh Log Message: ifconfig/t_capabilities: Skip unless run_unsafe is configured to yes The test modifies if_capabilities for all available interfaces. This is not a behavior we expect for normal ATF runs. s/if_capabilities/if_capenable/ here. Thanks, rin
Re: CVS commit: src/sys/dev/pci/ixgbe
On 2023/10/12 14:50, SAITOH Masanobu wrote: > Module Name: src > Committed By: msaitoh > Date: Thu Oct 12 05:50:56 UTC 2023 > > Modified Files: > src/sys/dev/pci/ixgbe: ixgbe.c > > Log Message: > ixg(4): Don't print wrong error message about ixgbe_num_queues. > > Don't override the ixgbe_num_queues global variable. It's the default > value of the number of queues and should not override it because it > will be referenced by later device attach. For example, the number of > MSI-X vector is 64 on X540 and 18 on 82599. When both cards are inserted > to a machine that the number of CPU is 24 and X540 is probed earlier, > ixgbe_num_queues is overridden to 24 and the following error message is > printed when attaching 82599: > > ixg2: autoconfiguration error: ixgbe_num_queues (24) is too large, > using reduced amount (17). > > Note that the number of queues is in sc->num_queuss and referenced > by hw.ixgN.num_queues sysctl. The commit message was incorrect. - s/82599/82598/ - Worse thing can happen if a smaller number of MSI-X vector's device is attached earlier. The small number is set as the default value and the number of queues of the next device is unintentionally limited to it. > To generate a diff of this commit: > cvs rdiff -u -r1.341 -r1.342 src/sys/dev/pci/ixgbe/ixgbe.c > > Please note that diffs are not public domain; they are subject to the > copyright notices on the relevant files. > -- --- SAITOH Masanobu (msai...@execsw.org msai...@netbsd.org)
Re: CVS commit: src/sys/dev/pci/igc
On 2023-10-15 17.06, Joerg Sonnenberger wrote: On Sun, Oct 15, 2023 at 10:36:53PM +, Greg Oster wrote: Module Name:src Committed By: oster Date: Sun Oct 15 22:36:53 UTC 2023 Modified Files: src/sys/dev/pci/igc: if_igc.c Log Message: Fix build of the MODULAR kernel, which explicitly excludes vlans. Please fix the macro to not expand to nothing instead. I think you're referring to the change I made to src/sys/dev/sequencer.c instead of this one... Done! Later... Greg Oster
Re: CVS commit: src/sys/dev/pci/igc
On Sun, Oct 15, 2023 at 10:36:53PM +, Greg Oster wrote: > Module Name: src > Committed By: oster > Date: Sun Oct 15 22:36:53 UTC 2023 > > Modified Files: > src/sys/dev/pci/igc: if_igc.c > > Log Message: > Fix build of the MODULAR kernel, which explicitly excludes vlans. Please fix the macro to not expand to nothing instead. Joerg
Re: CVS commit: src/sys
> Date: Fri, 13 Oct 2023 19:03:35 + > From: Andrew Doran > > On Thu, Oct 12, 2023 at 11:55:46AM +0200, J. Hannken-Illjes wrote: > > This is not true for RUMP. Hero you added sleepq_remove() as > > "sleepq_unsleep(l, true)". This will unlock l_mutex without changing. > > > > Just poking around and using sleepq_unsleep(l, false) here makes the > > NFS tests using rump_server pass. > > Ah, now I see, thank you. I committed a fix and will do a test run once my > build completes. Thanks, testbed looks much better now!
re: CVS commit: src/lib/libc/stdlib
> Minor changes to jemalloc100 (the old one that only vax etc currently uses). thanks. i'm still using this version on a bunch of modern machines. new jemalloc was problematic for a few things for me a number of years ago and i keep meaning to test again, but for now i'm still mostly using this version everwhere. FYI. .mrg.
Re: CVS commit: src/sys
On Thu, Oct 12, 2023 at 11:55:46AM +0200, J. Hannken-Illjes wrote: > > On 10. Oct 2023, at 20:58, Andrew Doran wrote: > > > > On Tue, Oct 10, 2023 at 06:00:57PM +0200, J. Hannken-Illjes wrote: > > > >>> cvs rdiff -u -r1.63 -r1.64 src/sys/kern/sys_select.c > >> > >> -sleepq_unsleep(l, false); > >> +sleepq_remove(l->l_sleepq, l, true); > >>} > >> } > >> mutex_spin_exit(lock); > >> > >> Looks like sleepq_remove() unlocks l->l_mutex == lock and > >> then mutex_spin_exit(lock) will unlock an unlocked mutex. > > > > lock is held before the call to sleepq_remove() and this is also true at the > > time: l->l_mutex == lock. > > > > After the call lock is still held, but now l->l_mutex != lock, because l has > > changed state (e.g LSSLEEP -> LSRUN) which causes l_mutex to change in > > concert. There is a rough overview here: > > > > https://nxr.netbsd.org/xref/src/sys/kern/kern_lwp.c#156 > > > > Did you encounter a problem? > > This is not true for RUMP. Hero you added sleepq_remove() as > "sleepq_unsleep(l, true)". This will unlock l_mutex without changing. > > Just poking around and using sleepq_unsleep(l, false) here makes the > NFS tests using rump_server pass. Ah, now I see, thank you. I committed a fix and will do a test run once my build completes. Cheers, Andrew
Re: CVS commit: src/sys/kern
> Date: Fri, 13 Oct 2023 17:52:07 +0900 > From: Rin Okuyama > > It would be really nice if we can find some systematical/reliable methods to > figure out files that really depends on struct syncobj, e.g.. I tried > ctfdump(1) to > *.o for kernel modules, but I cannot extract information better than > ``grep syncobj.h .depend''... The attached patch removes all use of sys/syncobj.h outside .c files under sys, so we can be reasonably confident userland programs -- except for crash(8), which is kind of special -- are unaffected. However, I'm going to hold off on committing this until the tree's sleepq issues are fixed so our testbed can run again. >From 7e9e2af19ecc6f4262b928da8a97a49d171c8072 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Fri, 13 Oct 2023 11:04:20 + Subject: [PATCH] sys/lwp.h: Nix sys/syncobj.h dependency. Remove it in ddb/db_syncobj.h too. New sys/wchan.h defines wchan_t so that users need not pull in sys/syncobj.h to get it. Sprinkle #include in .c files where it is now needed. --- sys/ddb/db_syncobj.h | 2 +- sys/kern/kern_condvar.c | 1 + sys/kern/kern_ktrace.c| 1 + sys/kern/kern_lwp.c | 1 + sys/kern/kern_mutex.c | 1 + sys/kern/kern_rwlock.c| 1 + sys/kern/kern_sleepq.c| 1 + sys/kern/kern_turnstile.c | 1 + sys/kern/sys_lwp.c| 1 + sys/kern/sys_select.c | 1 + sys/sys/lwp.h | 2 +- sys/sys/sleepq.h | 2 +- sys/sys/sleeptab.h| 6 +- sys/sys/syncobj.h | 4 ++-- sys/sys/wchan.h | 37 + 15 files changed, 56 insertions(+), 6 deletions(-) create mode 100644 sys/sys/wchan.h diff --git a/sys/ddb/db_syncobj.h b/sys/ddb/db_syncobj.h index 2c2ad89ba177..dc7594f5163e 100644 --- a/sys/ddb/db_syncobj.h +++ b/sys/ddb/db_syncobj.h @@ -29,7 +29,7 @@ #ifndef_DDB_DB_SYNCOBJ_H #define_DDB_DB_SYNCOBJ_H -#include +#include struct lwp; struct syncobj; diff --git a/sys/kern/kern_condvar.c b/sys/kern/kern_condvar.c index 478c4a35ff2b..c25282e1beb3 100644 --- a/sys/kern/kern_condvar.c +++ b/sys/kern/kern_condvar.c @@ -45,6 +45,7 @@ __KERNEL_RCSID(0, "$NetBSD: kern_condvar.c,v 1.59 2023/10/12 23:51:05 ad Exp $") #include #include #include +#include /* * Accessors for the private contents of the kcondvar_t data type. diff --git a/sys/kern/kern_ktrace.c b/sys/kern/kern_ktrace.c index 5ad5272af7d8..812be0c2c9ca 100644 --- a/sys/kern/kern_ktrace.c +++ b/sys/kern/kern_ktrace.c @@ -77,6 +77,7 @@ __KERNEL_RCSID(0, "$NetBSD: kern_ktrace.c,v 1.182 2022/07/01 01:07:56 riastradh #include #include #include +#include #include #include diff --git a/sys/kern/kern_lwp.c b/sys/kern/kern_lwp.c index 77e43242f0f9..971e0180f1f6 100644 --- a/sys/kern/kern_lwp.c +++ b/sys/kern/kern_lwp.c @@ -253,6 +253,7 @@ __KERNEL_RCSID(0, "$NetBSD: kern_lwp.c,v 1.265 2023/10/05 19:41:06 ad Exp $"); #include #include #include +#include #include #include diff --git a/sys/kern/kern_mutex.c b/sys/kern/kern_mutex.c index 810ea121a0bd..640909bc533e 100644 --- a/sys/kern/kern_mutex.c +++ b/sys/kern/kern_mutex.c @@ -57,6 +57,7 @@ __KERNEL_RCSID(0, "$NetBSD: kern_mutex.c,v 1.110 2023/09/23 18:48:04 ad Exp $"); #include #include #include +#include #include diff --git a/sys/kern/kern_rwlock.c b/sys/kern/kern_rwlock.c index 88db7d507b4d..96312874a069 100644 --- a/sys/kern/kern_rwlock.c +++ b/sys/kern/kern_rwlock.c @@ -62,6 +62,7 @@ __KERNEL_RCSID(0, "$NetBSD: kern_rwlock.c,v 1.74 2023/10/04 20:39:35 ad Exp $"); #include #include #include +#include #include diff --git a/sys/kern/kern_sleepq.c b/sys/kern/kern_sleepq.c index e9d39445f75b..bb43e78f6f6b 100644 --- a/sys/kern/kern_sleepq.c +++ b/sys/kern/kern_sleepq.c @@ -49,6 +49,7 @@ __KERNEL_RCSID(0, "$NetBSD: kern_sleepq.c,v 1.83 2023/10/08 13:37:26 ad Exp $"); #include #include #include +#include /* * for sleepq_abort: diff --git a/sys/kern/kern_turnstile.c b/sys/kern/kern_turnstile.c index 0cd8886cb6b5..85bdf946c325 100644 --- a/sys/kern/kern_turnstile.c +++ b/sys/kern/kern_turnstile.c @@ -68,6 +68,7 @@ __KERNEL_RCSID(0, "$NetBSD: kern_turnstile.c,v 1.53 2023/10/08 13:23:05 ad Exp $ #include #include #include +#include #include /* diff --git a/sys/kern/sys_lwp.c b/sys/kern/sys_lwp.c index c71cf1e823d6..108d40641e38 100644 --- a/sys/kern/sys_lwp.c +++ b/sys/kern/sys_lwp.c @@ -51,6 +51,7 @@ __KERNEL_RCSID(0, "$NetBSD: sys_lwp.c,v 1.87 2023/10/08 13:23:05 ad Exp $"); #include #include #include +#include #include diff --git a/sys/kern/sys_select.c b/sys/kern/sys_select.c index 9719d220e319..16962505663c 100644 --- a/sys/kern/sys_select.c +++ b/sys/kern/sys_select.c @@ -106,6 +106,7 @@ __KERNEL_RCSID(0, "$NetBSD: sys_select.c,v 1.64 2023/10/08 13:23:05 ad Exp $"); #include #include #include +#include /* Flags for lwp::l_selflag. */ #defineSEL_RESET 0 /* awoken, interrupted, or not yet
Re: CVS commit: src/sys
> Date: Tue, 10 Oct 2023 18:58:29 + > From: Andrew Doran > > On Tue, Oct 10, 2023 at 06:00:57PM +0200, J. Hannken-Illjes wrote: > > > > cvs rdiff -u -r1.63 -r1.64 src/sys/kern/sys_select.c > > > > -sleepq_unsleep(l, false); > > +sleepq_remove(l->l_sleepq, l, true); > > } > >} > >mutex_spin_exit(lock); > > > > Looks like sleepq_remove() unlocks l->l_mutex == lock and > > then mutex_spin_exit(lock) will unlock an unlocked mutex. > [...] > Did you encounter a problem? Yes, this appears to be breaking hundreds of automatic tests, which is getting in the way of all kernel testing. https://mail-index.netbsd.org/current-users/2023/10/13/msg044522.html https://mail-index.netbsd.org/current-users/2023/10/13/msg044523.html https://mail-index.netbsd.org/current-users/2023/10/13/msg044524.html Enough breakage has happened over several days that bracket is unable to discern which changes have broken which tests now: https://mail-index.netbsd.org/current-users/2023/10/13/msg044525.html https://mail-index.netbsd.org/current-users/2023/10/13/msg044526.html Please fix immediately or back out.
Re: CVS commit: src/sys/kern
On Thu, Oct 12, 2023 at 8:23 PM Taylor R Campbell wrote: > > > Date: Thu, 12 Oct 2023 17:06:02 +0900 > > From: Rin Okuyama > > > > On Thu, Oct 5, 2023 at 5:39 AM Andrew Doran wrote: > > > > > > Module Name:src > > > Committed By: ad > > > Date: Wed Oct 4 20:39:35 UTC 2023 > > > > > > Modified Files: > > > src/sys/kern: kern_rwlock.c kern_turnstile.c > > > > > > Log Message: > > > Turnstiles: use the syncobj name for ps/top wmesg when sleeping since it's > > > more informative than "tstile". > > > > Cool! Can I send a pull up request to netbsd-10? > > Not sure -- it would depend on this commit to introduce struct > syncobj::sobj_name: > > https://mail-index.netbsd.org/source-changes/2023/07/17/msg146058.html > > This is a potential kernel ABI change. I didn't investigate to > determine whether it would be safe to pull up. Thanks, I didn't notice that. It should be too risky to pull these up just before RC1. I withdraw this proposal. PS It would be really nice if we can find some systematical/reliable methods to figure out files that really depends on struct syncobj, e.g.. I tried ctfdump(1) to *.o for kernel modules, but I cannot extract information better than ``grep syncobj.h .depend''... Thanks, rin
Re: CVS commit: src/sys/kern
> Date: Thu, 12 Oct 2023 17:06:02 +0900 > From: Rin Okuyama > > On Thu, Oct 5, 2023 at 5:39â¯AM Andrew Doran wrote: > > > > Module Name:src > > Committed By: ad > > Date: Wed Oct 4 20:39:35 UTC 2023 > > > > Modified Files: > > src/sys/kern: kern_rwlock.c kern_turnstile.c > > > > Log Message: > > Turnstiles: use the syncobj name for ps/top wmesg when sleeping since it's > > more informative than "tstile". > > Cool! Can I send a pull up request to netbsd-10? Not sure -- it would depend on this commit to introduce struct syncobj::sobj_name: https://mail-index.netbsd.org/source-changes/2023/07/17/msg146058.html This is a potential kernel ABI change. I didn't investigate to determine whether it would be safe to pull up.
Re: CVS commit: src/sys
> On 10. Oct 2023, at 20:58, Andrew Doran wrote: > > On Tue, Oct 10, 2023 at 06:00:57PM +0200, J. Hannken-Illjes wrote: > >>> cvs rdiff -u -r1.63 -r1.64 src/sys/kern/sys_select.c >> >> -sleepq_unsleep(l, false); >> +sleepq_remove(l->l_sleepq, l, true); >>} >> } >> mutex_spin_exit(lock); >> >> Looks like sleepq_remove() unlocks l->l_mutex == lock and >> then mutex_spin_exit(lock) will unlock an unlocked mutex. > > lock is held before the call to sleepq_remove() and this is also true at the > time: l->l_mutex == lock. > > After the call lock is still held, but now l->l_mutex != lock, because l has > changed state (e.g LSSLEEP -> LSRUN) which causes l_mutex to change in > concert. There is a rough overview here: > > https://nxr.netbsd.org/xref/src/sys/kern/kern_lwp.c#156 > > Did you encounter a problem? This is not true for RUMP. Hero you added sleepq_remove() as "sleepq_unsleep(l, true)". This will unlock l_mutex without changing. Just poking around and using sleepq_unsleep(l, false) here makes the NFS tests using rump_server pass. -- J. Hannken-Illjes - hann...@mailbox.org signature.asc Description: Message signed with OpenPGP
Re: CVS commit: src/sys/kern
Cool! Can I send a pull up request to netbsd-10? I've not yet observed deadlocks since this was committed, fortunately or unfortunately, although ;) Thanks, rin On Thu, Oct 5, 2023 at 5:39 AM Andrew Doran wrote: > > Module Name:src > Committed By: ad > Date: Wed Oct 4 20:39:35 UTC 2023 > > Modified Files: > src/sys/kern: kern_rwlock.c kern_turnstile.c > > Log Message: > Turnstiles: use the syncobj name for ps/top wmesg when sleeping since it's > more informative than "tstile". > > > To generate a diff of this commit: > cvs rdiff -u -r1.73 -r1.74 src/sys/kern/kern_rwlock.c > cvs rdiff -u -r1.51 -r1.52 src/sys/kern/kern_turnstile.c > > Please note that diffs are not public domain; they are subject to the > copyright notices on the relevant files. >
Re: CVS commit: src/sys
On Tue, Oct 10, 2023 at 06:00:57PM +0200, J. Hannken-Illjes wrote: > > cvs rdiff -u -r1.63 -r1.64 src/sys/kern/sys_select.c > > -sleepq_unsleep(l, false); > +sleepq_remove(l->l_sleepq, l, true); > } >} >mutex_spin_exit(lock); > > Looks like sleepq_remove() unlocks l->l_mutex == lock and > then mutex_spin_exit(lock) will unlock an unlocked mutex. lock is held before the call to sleepq_remove() and this is also true at the time: l->l_mutex == lock. After the call lock is still held, but now l->l_mutex != lock, because l has changed state (e.g LSSLEEP -> LSRUN) which causes l_mutex to change in concert. There is a rough overview here: https://nxr.netbsd.org/xref/src/sys/kern/kern_lwp.c#156 Did you encounter a problem? Cheers, Andrew
Re: CVS commit: src/sys
> On 8. Oct 2023, at 15:23, Andrew Doran wrote: > > Module Name: src > Committed By: ad > Date: Sun Oct 8 13:23:05 UTC 2023 > > Modified Files: > src/sys/kern: kern_condvar.c kern_sleepq.c kern_timeout.c >kern_turnstile.c sys_lwp.c sys_select.c > src/sys/rump/librump/rumpkern: sleepq.c > src/sys/sys: sleepq.h syncobj.h > > Log Message: > Ensure that an LWP that has taken a legitimate wakeup never produces an > error code from sleepq_block(). Then, it's possible to make cv_signal() > work as expected and only ever wake a singular LWP. > > > To generate a diff of this commit: ... > cvs rdiff -u -r1.63 -r1.64 src/sys/kern/sys_select.c -sleepq_unsleep(l, false); +sleepq_remove(l->l_sleepq, l, true); } } mutex_spin_exit(lock); Looks like sleepq_remove() unlocks l->l_mutex == lock and then mutex_spin_exit(lock) will unlock an unlocked mutex. -- J. Hannken-Illjes - hann...@mailbox.org signature.asc Description: Message signed with OpenPGP
Re: CVS commit: src/sys/arch/news68k/conf
OK, it makes sense. I will revert the changes. Thanks for your explanations. On Sun, Oct 8, 2023 at 12:49 PM matthew green wrote: > > > I was changing news68k specific code, thus wasn't treating them as > > common. But I understand the point. > > there's a *LOT* of m68k code that is copied into all the ports > that is almost identical, and should really be shared, but it > not, and changes like can make this harder to share. > > ie, while it might appear news68k specific, ideally most of it > would end up in arch/m68k instead. > > it would really be far better for that merge, than to worry > about obscure compile options -- there's a real cost when we > have platform changes to make, and 10 m68k copies are needed > instead of 1. we've done some of this over time (for not > just m68k) but there's quite a lot of left here.. > > thanks. > > > .mrg.
re: CVS commit: src/sys/arch/news68k/conf
> I was changing news68k specific code, thus wasn't treating them as > common. But I understand the point. there's a *LOT* of m68k code that is copied into all the ports that is almost identical, and should really be shared, but it not, and changes like can make this harder to share. ie, while it might appear news68k specific, ideally most of it would end up in arch/m68k instead. it would really be far better for that merge, than to worry about obscure compile options -- there's a real cost when we have platform changes to make, and 10 m68k copies are needed instead of 1. we've done some of this over time (for not just m68k) but there's quite a lot of left here.. thanks. .mrg.
Re: CVS commit: src/sys/arch/news68k/conf
On Sun, Oct 8, 2023 at 6:56 AM Izumi Tsutsui wrote:
>
> > In this case maybe I should remove all FPSP references (vectors.S,
> > locore.S, Makefile.news68k (MD_LIBS={FPSP})?
>
> IMO we don't have to keep strict consistencies or buildabilities of
> options but rather should consider readabilities and maintainabilities.
>
> - options FPSP in a config file is not necessary for users on news68k
> (unless some users build own 040/060 upgrade mod like x68k)
Yes, that's understandable and true. However, if someone would have a
mod, he would need to understand why FPSP build fails in the first
place.
I just wanted to avoid confusion, that code has ifdef blocks, but
there's no way to build it.
> - #ifdef FPSP (and other m68k specific options) blocks in common
> sources might help to check diffs from other m68k ports
I was changing news68k specific code, thus wasn't treating them as
common. But I understand the point.
I can return FPSP block in vectors.s, if that helps with readability
and maintainability.
However, those copy pasted common files usually diverge between ports
(from what I've seen),
makes it difficult to understand what is still common and what is the
"latest" code at times (likely not in this case).
> - In the perfect world, we should have a common m68k/locore.s etc.
>and move port specific blocks into locore_machdep.s like mips,
>but m68k CPUs are too flexible and all m68k ports have too many
>historical difficulties to refactor them
I admit I went a bit impatient this time without waiting for the
proper answer. I am OK to settle with the best option in the context,
either revert everything to original state, keep partial changes or
leave current state. Sorry for this.
>
> I don't mind your changes in this case, but it's appreciated
> to ask design considerations before changes, as you did for mmeye.
>
> Thanks,
> ---
> Izumi Tsutsui
Re: CVS commit: src/sys/arch/news68k/conf
> In this case maybe I should remove all FPSP references (vectors.S,
> locore.S, Makefile.news68k (MD_LIBS={FPSP})?
IMO we don't have to keep strict consistencies or buildabilities of
options but rather should consider readabilities and maintainabilities.
- options FPSP in a config file is not necessary for users on news68k
(unless some users build own 040/060 upgrade mod like x68k)
- #ifdef FPSP (and other m68k specific options) blocks in common
sources might help to check diffs from other m68k ports
- In the perfect world, we should have a common m68k/locore.s etc.
and move port specific blocks into locore_machdep.s like mips,
but m68k CPUs are too flexible and all m68k ports have too many
historical difficulties to refactor them
I don't mind your changes in this case, but it's appreciated
to ask design considerations before changes, as you did for mmeye.
Thanks,
---
Izumi Tsutsui
Re: CVS commit: src/sys
On Thu, Oct 05, 2023 at 12:15:23PM +0200, Martin Husemann wrote: > On Thu, Oct 05, 2023 at 09:59:49AM +, Andrew Doran wrote: > > Yes that makes sense and is what I plan to do after work today if nobody > > beats me to it. MULTIPROCESSOR is long overdue removal from the MI kernel, > > IMO. > > Hey, this is a tiny landisk kernel, do not bloat it :-) And I thought I had more of a reputation for tearing things out of the OS.. > Unfortunately it is not trivial to make that zeroing happen in the > macros that we have now (gcc is pretty picky with warnings, would love > to have c++ templates here). I rearranged things so it shouldn't be necessary.. At least a pmax kernel builds for me now. Sorry about the interruption. Andrew
Re: CVS commit: src/sys
On Thu, Oct 05, 2023 at 09:59:49AM +, Andrew Doran wrote: > Yes that makes sense and is what I plan to do after work today if nobody > beats me to it. MULTIPROCESSOR is long overdue removal from the MI kernel, > IMO. Hey, this is a tiny landisk kernel, do not bloat it :-) Unfortunately it is not trivial to make that zeroing happen in the macros that we have now (gcc is pretty picky with warnings, would love to have c++ templates here). Martin
Re: CVS commit: src/sys
Martin, On Thu, Oct 05, 2023 at 11:42:03AM +0200, Martin Husemann wrote: > On Thu, Oct 05, 2023 at 11:36:23AM +0200, Martin Husemann wrote: > > No, I was confused by the #ifdef maze - it breaks the build for > > non-MULTIPROCESSOR kernels only, and I am not actually sure what "use" > > gcc sees for the "nlocks" variable at all in that case. > > Scratch that too, I'll get coffee. > > Andrew, should we make the non-MULTIPROCESSOR variant of > > KERNEL_UNLOCK(all, lwp, ptr) > > set *(ptr) = 0 ? Yes that makes sense and is what I plan to do after work today if nobody beats me to it. MULTIPROCESSOR is long overdue removal from the MI kernel, IMO. Andrew
Re: CVS commit: src/sys
On Thu, Oct 05, 2023 at 11:36:23AM +0200, Martin Husemann wrote: > No, I was confused by the #ifdef maze - it breaks the build for > non-MULTIPROCESSOR kernels only, and I am not actually sure what "use" > gcc sees for the "nlocks" variable at all in that case. Scratch that too, I'll get coffee. Andrew, should we make the non-MULTIPROCESSOR variant of KERNEL_UNLOCK(all, lwp, ptr) set *(ptr) = 0 ? Martin
Re: CVS commit: src/sys
On Thu, Oct 05, 2023 at 11:28:49AM +0200, Martin Husemann wrote: > This breaks the build: > > ../../../../kern/kern_synch.c: In function 'kpause': > ../../../../kern/kern_synch.c:257:10: error: 'nlocks' may be used > uninitialized in this function [-Werror=maybe-uninitialized] > 257 | error = sleepq_block(timo, intr, _syncobj, nlocks); > | ^ > cc1: all warnings being treated as errors > > > and it looks like gcc is totally correct here: KERNEL_UNLOCK_ALL() > does not set nlocks but KERNEL_LOCK() later KASSERTS it is > 0. No, I was confused by the #ifdef maze - it breaks the build for non-MULTIPROCESSOR kernels only, and I am not actually sure what "use" gcc sees for the "nlocks" variable at all in that case. Martin
Re: CVS commit: src/sys
On Wed, Oct 04, 2023 at 08:29:18PM +, Andrew Doran wrote: > Module Name: src > Committed By: ad > Date: Wed Oct 4 20:29:18 UTC 2023 > > Modified Files: > src/sys/kern: kern_condvar.c kern_exec.c kern_exit.c kern_sig.c > kern_sleepq.c kern_synch.c kern_timeout.c kern_turnstile.c > sys_lwp.c sys_select.c > src/sys/rump/librump/rumpkern: lwproc.c sleepq.c > src/sys/sys: lwp.h sleepq.h > > Log Message: > Eliminate l->l_biglocks. Originally I think it had a use but these days a > local variable will do. This breaks the build: ../../../../kern/kern_synch.c: In function 'kpause': ../../../../kern/kern_synch.c:257:10: error: 'nlocks' may be used uninitialized in this function [-Werror=maybe-uninitialized] 257 | error = sleepq_block(timo, intr, _syncobj, nlocks); | ^ cc1: all warnings being treated as errors and it looks like gcc is totally correct here: KERNEL_UNLOCK_ALL() does not set nlocks but KERNEL_LOCK() later KASSERTS it is > 0. Martin
Re: CVS commit: src/sys/arch/news68k/conf
In this case maybe I should remove all FPSP references (vectors.S,
locore.S, Makefile.news68k (MD_LIBS={FPSP})?
On Sun, Oct 1, 2023 at 10:08 PM Izumi Tsutsui wrote:
>
> > Module Name: src
> > Committed By: andvar
> > Date: Sun Oct 1 18:50:53 UTC 2023
> >
> > Modified Files:
> > src/sys/arch/news68k/conf: Makefile.news68k
> >
> > Log Message:
> > include fpsp Makefile.inc in Makefile.news68k, same as other m68k ports.
> >
> > needed for FPSP option to build, otherwise FPSP specific vectors are
> > undefined.
>
> FPSP is neccesary only for 68040 (and 68060) and
> there is no 040 NEWS machines. That's the reason
> why news68k doesn't include fpsp.
>
> ---
> Izumi Tsutsui
Re: CVS commit: src/sys/arch/news68k/conf
> Module Name: src > Committed By: andvar > Date: Sun Oct 1 18:50:53 UTC 2023 > > Modified Files: > src/sys/arch/news68k/conf: Makefile.news68k > > Log Message: > include fpsp Makefile.inc in Makefile.news68k, same as other m68k ports. > > needed for FPSP option to build, otherwise FPSP specific vectors are > undefined. FPSP is neccesary only for 68040 (and 68060) and there is no 040 NEWS machines. That's the reason why news68k doesn't include fpsp. --- Izumi Tsutsui
Re: CVS commit: src/sbin/gpt
Date:Wed, 27 Sep 2023 09:44:10 + From:"Taylor R Campbell" Message-ID: <20230927094410.b9257f...@cvs.netbsd.org> | gpt(8): Make gpt type array and enum match again. Thanks, and apologies for not checking that better - I did test that it recognised the new one properly... kre
Re: CVS commit: src
On Sun, Sep 24, 2023 at 03:51:07AM +0900, Rin Okuyama wrote:
> Hi,
>
> On 2023/09/24 3:21, Andrew Doran wrote:
> > Index: src/common/lib/libc/gen/radixtree.c
> > diff -u src/common/lib/libc/gen/radixtree.c:1.31
> > src/common/lib/libc/gen/radixtree.c:1.32
> > --- src/common/lib/libc/gen/radixtree.c:1.31Tue Sep 12 16:17:21 2023
> > +++ src/common/lib/libc/gen/radixtree.c Sat Sep 23 18:21:11 2023
> ...
> > @@ -346,10 +331,11 @@ radix_tree_await_memory(void)
> > int i;
> > for (i = 0; i < __arraycount(nodes); i++) {
> > - nodes[i] = pool_cache_get(radix_tree_node_cache, PR_WAITOK);
> > + nodes[i] = kmem_intr_alloc(sizeof(struct radix_tree_node),
> > + KM_SLEEP);
> > }
> > while (--i >= 0) {
> > - pool_cache_put(radix_tree_node_cache, nodes[i]);
> > + kmem_free(nodes[i], sizeof(struct radix_tree_node));
> > }
> > }
>
> kmem_intr_free() here?
Good catch! Thank you.
Andrew
Re: CVS commit: src
Hi,
On 2023/09/24 3:21, Andrew Doran wrote:
Index: src/common/lib/libc/gen/radixtree.c
diff -u src/common/lib/libc/gen/radixtree.c:1.31
src/common/lib/libc/gen/radixtree.c:1.32
--- src/common/lib/libc/gen/radixtree.c:1.31Tue Sep 12 16:17:21 2023
+++ src/common/lib/libc/gen/radixtree.c Sat Sep 23 18:21:11 2023
...
@@ -346,10 +331,11 @@ radix_tree_await_memory(void)
int i;
for (i = 0; i < __arraycount(nodes); i++) {
- nodes[i] = pool_cache_get(radix_tree_node_cache, PR_WAITOK);
+ nodes[i] = kmem_intr_alloc(sizeof(struct radix_tree_node),
+ KM_SLEEP);
}
while (--i >= 0) {
- pool_cache_put(radix_tree_node_cache, nodes[i]);
+ kmem_free(nodes[i], sizeof(struct radix_tree_node));
}
}
kmem_intr_free() here?
Thanks,
rin
Re: CVS commit: src/external/gpl3/binutils/dist/ld/emultempl
On Mon, Sep 11, 2023 at 12:27:25PM +1000, matthew green wrote: > "Rin Okuyama" writes: > > Module Name:src > > Committed By: rin > > Date: Mon Sep 11 01:54:18 UTC 2023 > > > > Modified Files: > > src/external/gpl3/binutils/dist/ld/emultempl: aarch64elf.em armelf.em > > elf.em > > > > Log Message: > > ld: Enable --copy-dt-needed-entries by default again > > thanks for fixing -lcurses. > > can we put this into bsd.*.mk instead? > > ie, if we want this, declare it explicitly, rather than > reply upon patched binutils? (this is eg, unfriendly to > EXTERNAL_TOOLCHAIN etc.) We (TNF) consider this the default a bug. Red Hat etc who've been pushing for this mess don't, for reasons that don't really make sense. Basically, the Linux toolchain folks decided that ELF should no longer be recursive except it still is for every use case were performance matters. Joerg
Re: CVS commit: src/external/gpl3/binutils/dist/ld/emultempl
On 2023/09/11 11:27, matthew green wrote: "Rin Okuyama" writes: Module Name:src Committed By: rin Date: Mon Sep 11 01:54:18 UTC 2023 Modified Files: src/external/gpl3/binutils/dist/ld/emultempl: aarch64elf.em armelf.em elf.em Log Message: ld: Enable --copy-dt-needed-entries by default again thanks for fixing -lcurses. can we put this into bsd.*.mk instead? ie, if we want this, declare it explicitly, rather than reply upon patched binutils? (this is eg, unfriendly to EXTERNAL_TOOLCHAIN etc.) if it works, of course Thanks for your comment. Yeah, I feel this fix is too much also... I've found that FreeBSD uses ldscript to handle curses vs terminfo problem. I think we can resolve the problem in a similar manner. I've sent PR lib/57615 for this: http://gnats.netbsd.org/57615 Thanks, rin
re: CVS commit: src/external/gpl3/binutils/dist/ld/emultempl
"Rin Okuyama" writes: > Module Name: src > Committed By: rin > Date: Mon Sep 11 01:54:18 UTC 2023 > > Modified Files: > src/external/gpl3/binutils/dist/ld/emultempl: aarch64elf.em armelf.em > elf.em > > Log Message: > ld: Enable --copy-dt-needed-entries by default again thanks for fixing -lcurses. can we put this into bsd.*.mk instead? ie, if we want this, declare it explicitly, rather than reply upon patched binutils? (this is eg, unfriendly to EXTERNAL_TOOLCHAIN etc.) if it works, of course .mrg.
Re: CVS commit: src/usr.bin/make
On Fri, Sep 8, 2023 at 21:38 matthew green wrote: > Module Name:src > Committed By: mrg > Date: Sat Sep 9 04:38:49 UTC 2023 > > Modified Files: > src/usr.bin/make: main.c > > Log Message: > add explicit cast for long -> int truncation warning-as-error. > > as this is number of CPUs, i don't think we have to care about > it for a long, long, *long* time... Pun intended? > > > To generate a diff of this commit: > cvs rdiff -u -r1.594 -r1.595 src/usr.bin/make/main.c > > Please note that diffs are not public domain; they are subject to the > copyright notices on the relevant files. > >
Re: CVS commit: src/sys/arch/amiga/conf
Hi, Oops, I didn't notice it is not generated. Sorry for bothering you! Thanks, rin On Mon, Sep 4, 2023 at 10:46 PM Andrius V wrote: > > Hi, > > Thanks for the note. I honestly wasn't aware that we have generated > configs and will be more careful in changing them in the future to > account that. > > However, MDINSTALL config seems to be some older/legacy config, which > was not generated from GENERIC.in, I believe. I may try to transform > into generated one though, if there's a need for that. I guess RAMDISK > has somewhat similar purpose. > > > On Mon, Sep 4, 2023 at 12:17 PM Rin Okuyama wrote: > > > > Hi, > > > > On 2023/08/31 5:17, Andrius Varanavicius wrote: > > > Module Name: src > > > Committed By: andvar > > > Date: Wed Aug 30 20:17:06 UTC 2023 > > > > > > Modified Files: > > > src/sys/arch/amiga/conf: MDINSTALL > > > > > > Log Message: > > > s/Piccalo/Piccolo/ in device description. > > > > Some ports including amiga use arch/foo/conf/*.in to generate > > kernel config files. Please fix *.in and regen them. > > > > Thanks, > > rin
Re: CVS commit: src/external/gpl3/binutils/dist/bfd
On Tue, Sep 5, 2023 at 4:46 AM matthew green wrote:
>
> > I did similar verification for gdb/dist/bfd also. I'd like to
> > sync {binutils,gdb}/dist/bfd, but there are huge diffs between
> > them. Most of them seem like binutils or gdb specific fixes,
> > but I may overlook something...
> >
> > It must be nice if we could unify two libbfd's. The upstream
> > uses the same repository for binutils and gdb. However, the
> > release branches for them are quite different, unfortunately.
>
> we used to do this a long time and and it's really difficult
> to not break one toolchain component while updating another
> and we ditched the merged 'src' tree like upstream had.
>
> (long ago, GCC was in the same 'src' as well, but i think
> it's no longer the same. our merged tree had GCC too...)
>
> this is a nice idea, but practically we already stopped
> using it..
Ah, thanks for kind explanation. And,
> IFF we switched to importing gdb/binutils from the non
> release branch at the same point, we could probaly do this
> as long as we understand we're getting devel code, not
> release code, which is probably a bad idea...would like to
> have at least one of them as a release ;)
well, it should be painful ether way ;)
OK, I wil eventually send PRs to upstream to merge our
local changes eventually, at least until kamil@ gets some
time for NetBSD again :)
Thanks,
rin
re: CVS commit: src/external/gpl3/binutils/dist/bfd
> I did similar verification for gdb/dist/bfd also. I'd like to
> sync {binutils,gdb}/dist/bfd, but there are huge diffs between
> them. Most of them seem like binutils or gdb specific fixes,
> but I may overlook something...
>
> It must be nice if we could unify two libbfd's. The upstream
> uses the same repository for binutils and gdb. However, the
> release branches for them are quite different, unfortunately.
we used to do this a long time and and it's really difficult
to not break one toolchain component while updating another
and we ditched the merged 'src' tree like upstream had.
(long ago, GCC was in the same 'src' as well, but i think
it's no longer the same. our merged tree had GCC too...)
this is a nice idea, but practically we already stopped
using it..
IFF we switched to importing gdb/binutils from the non
release branch at the same point, we could probaly do this
as long as we understand we're getting devel code, not
release code, which is probably a bad idea...would like to
have at least one of them as a release ;)
.mrg.
Re: CVS commit: src/sys/arch/amiga/conf
Hi, Thanks for the note. I honestly wasn't aware that we have generated configs and will be more careful in changing them in the future to account that. However, MDINSTALL config seems to be some older/legacy config, which was not generated from GENERIC.in, I believe. I may try to transform into generated one though, if there's a need for that. I guess RAMDISK has somewhat similar purpose. On Mon, Sep 4, 2023 at 12:17 PM Rin Okuyama wrote: > > Hi, > > On 2023/08/31 5:17, Andrius Varanavicius wrote: > > Module Name: src > > Committed By: andvar > > Date: Wed Aug 30 20:17:06 UTC 2023 > > > > Modified Files: > > src/sys/arch/amiga/conf: MDINSTALL > > > > Log Message: > > s/Piccalo/Piccolo/ in device description. > > Some ports including amiga use arch/foo/conf/*.in to generate > kernel config files. Please fix *.in and regen them. > > Thanks, > rin
Re: CVS commit: src/sys/arch/amiga/conf
Hi, On 2023/08/31 5:17, Andrius Varanavicius wrote: Module Name:src Committed By: andvar Date: Wed Aug 30 20:17:06 UTC 2023 Modified Files: src/sys/arch/amiga/conf: MDINSTALL Log Message: s/Piccalo/Piccolo/ in device description. Some ports including amiga use arch/foo/conf/*.in to generate kernel config files. Please fix *.in and regen them. Thanks, rin
Re: CVS commit: src/external/gpl3/binutils/dist/bfd
On 2023/08/28 19:55, Valery Ushakov wrote:
On Mon, Aug 28, 2023 at 00:02:50 +, Rin Okuyama wrote:
Log Message:
binutils/bfd: Adjust blank line to reduce diff from upstream
Thanks a lot for these cleanups!
Do we need to apply similar cleanups to the bfd version in gdb?
(external/gpl3/gdb/dist/bfd)
Thanks!
I did similar verification for gdb/dist/bfd also. I'd like to
sync {binutils,gdb}/dist/bfd, but there are huge diffs between
them. Most of them seem like binutils or gdb specific fixes,
but I may overlook something...
It must be nice if we could unify two libbfd's. The upstream
uses the same repository for binutils and gdb. However, the
release branches for them are quite different, unfortunately.
For example, gdb 12 seems to be branched somewhere between
binutils 2.34 and 2.39, and they does not seem to merge non-
critical changes from development branch.
So, it is painful to unify both bfd's by our side. The best
choice we can make, should be to upstream our fixes, IMO...
Thanks,
rin
Re: CVS commit: src/sys/net
Thanks for kind words! Apparently, we need some test to detect kernel and userland mismatch for bpf(4) header... I'd also like to fix problems by which ATF does not complete on ERL3. Some CPU/memory consuming tests, e.g., lib/libc/regex/t_exhaust, trigger watchdog. I guess something wrong in interrupt handler for mips (IIRC, there are similar kind of behaviors for hpcmips machines). Disabling __HAVE_FAST_SOFTINT by hand does not work around the problem... For n32 userland, most rump-based tests in /usr/tests/net crash. But for n64 userland, on the other hand, they seem to work just fine. This may be due to bugs for n32 kernel. IIUC, most kernel codes are compiled for o32 or n64, and only the exception is n32 rump kernel for mips64e[bl] userland. Thanks, rin On 2023/08/26 9:10, David H. Gutteridge wrote: Module Name: src Committed By: rin Date: Wed Aug 23 13:21:17 UTC 2023 Modified Files: src/sys/net: bpf.h Log Message: bpf: Fix SIZEOF_BPF_HDR (for LP64 userland) on mips64 It cannot fit within 18 bytes, of course ;) As we had never provided working bpf(4) implementation for LP64 userland on mips, just use natural structure size here. Thanks for fixing this. This means dhcpcd now works on Octeon hardware using the n64 userland. Previously, it would fail with "ps_bpf_recvbpf: Resource temporarily unavailable". (Perhaps you'd noticed this?) This was reported a while ago[1] by an end user on port-mips@, so we should presumably pull this up. Where you say "we had never provided working bpf(4) implementation", I had been looking into the dhcpcd issue myself, but hadn't thought to look here. That is, I had run the tests under net/bpf and net/t_ip_reass (the latter modified to open /dev/bpf directly, not via rump) on my ERL3, and these all passed, giving me the impression we did indeed have a working bpf(4) implementation on n64. Since we didn't, it seems we need more test coverage here. Thanks, Dave 1. http://mail-index.netbsd.org/port-mips/2021/07/04/msg001112.html
re: CVS commit: src/external/gpl3/gcc.old/dist/gcc
"Christos Zoulas" writes: > Module Name: src > Committed By: christos > Date: Sun Sep 3 18:45:26 UTC 2023 > > Modified Files: > src/external/gpl3/gcc.old/dist/gcc: ubsan.c > > Log Message: > remap generated ubsan filename string labels. fixes reproducible builds. > reported by wiz@ thanks. surprised it was needed for .old, but not surprised the for GCC 12 -- that one had sanitizers replaced and i did not attempt to merge, but re-ported. it was pretty crazy -- our GCC 10 has a much nicer libsanitizer than GCC 10 upstream, but GCC 12 has upgraded to an even newer version that included many, but not all, of the netbsd fixes, and finding all the fixes from the prior is difficult, as there was no real "base version" to compare against parts of both GCC and llvm upstreams were that base.. anyway, i say this because there's always a chance of something else having been missed in the re-port. thanks again! .mrg.
Re: CVS commit: src/external/gpl3/binutils/dist/bfd
On Mon, Aug 28, 2023 at 00:02:50 +, Rin Okuyama wrote: > Log Message: > binutils/bfd: Adjust blank line to reduce diff from upstream Thanks a lot for these cleanups! Do we need to apply similar cleanups to the bfd version in gdb? (external/gpl3/gdb/dist/bfd) -uwe
Re: CVS commit: src/sys/net
Module Name:src Committed By: rin Date: Wed Aug 23 13:21:17 UTC 2023 Modified Files: src/sys/net: bpf.h Log Message: bpf: Fix SIZEOF_BPF_HDR (for LP64 userland) on mips64 It cannot fit within 18 bytes, of course ;) As we had never provided working bpf(4) implementation for LP64 userland on mips, just use natural structure size here. Thanks for fixing this. This means dhcpcd now works on Octeon hardware using the n64 userland. Previously, it would fail with "ps_bpf_recvbpf: Resource temporarily unavailable". (Perhaps you'd noticed this?) This was reported a while ago[1] by an end user on port-mips@, so we should presumably pull this up. Where you say "we had never provided working bpf(4) implementation", I had been looking into the dhcpcd issue myself, but hadn't thought to look here. That is, I had run the tests under net/bpf and net/t_ip_reass (the latter modified to open /dev/bpf directly, not via rump) on my ERL3, and these all passed, giving me the impression we did indeed have a working bpf(4) implementation on n64. Since we didn't, it seems we need more test coverage here. Thanks, Dave 1. http://mail-index.netbsd.org/port-mips/2021/07/04/msg001112.html
Re: CVS commit: src/sys/compat/linux/common
On 2023-08-25 13:30, Taylor R Campbell wrote: > Since VOP_READDIR requires vp to be locked, I can infer that the > handle_write caller must already hold vp locked. But that means that > we have ifd_lock -> vnode lock in one path, and vnode lock -> ifd_lock > in another path, which is forbidden (unless there's some reason we can > prove these paths never happen concurrently). Hmm... I did not realize this, so I don't think NOTE_WRITE disambiguation is (or has ever been) safe. The real pain here is that we inherit a held vp_interlock and vnode lock in the needs_lock=false case from the kevent callback. So this may require a pretty substantial locking revision(?) Anyways, I'll take a closer look later this weekend. > I'm also suspicious of logic like this: > > mutex_enter(>f_lock); > uio.uio_offset = fp->f_offset; > mutex_exit(>f_lock); > ... > mutex_enter(>f_lock); > fp->f_offset = uio.uio_offset; > mutex_exit(>f_lock); > > If fp->f_offset can change concurrently while f_lock is released, this > looks like a problem. But if it can't, why do we need the lock at > all? I suspect this really does need a lock over the whole > transaction (maybe fp->f_lock, maybe something else), which is not > released while I/O is happening -- possibly not with mutex(9) but > something interruptible instead. I think you're right about needing a lock for the whole transaction. Since this is called from get_inotify_dir_entries(), perhaps it would be better for get_inotify_dir_entries() to keep track of the offset and pass it in instead? Theo(dore)
Re: CVS commit: src/sys/compat/linux/common
> Date: Fri, 25 Aug 2023 13:38:02 -0400 > From: Theodore Preduta > > On 2023-08-25 13:13, Taylor R Campbell wrote: > > This can't be right, and it's a little unsettling that the problem > > isn't caught by any automatic tests. > > > > As I understand it, the `ie_name' member is supposed to provide > > storage for the `ie_event.name' array. > > > > So this looks like this change is going to lead either to a buffer > > overrun -- reading someone else's heap memory, or scribbling all over > > someone else's heap memory -- or to uninitialized or zero-filled > > memory being published to userland where there should be a pathname. > > I don't think so. Notice that in inotify_read() two calls to uiomove() > are made, one on ie_event and one on ie_name... and in general this code > never accesses ie_event.name directly. > > The reason I separated the fields in the first place was to make > allocation/deallocation simpler, not to use ie_name as storage for > ie_event.name. Got it! So that would explain why automatic tests didn't catch anything. Thanks! Maybe we should have a comment about this to clarify for future readers. Side note: I realize there's a KASSERT protecting one of the strcpy calls, and the other one is limited to copying from values of struct dirent::d_name created by VOP_READDIR which should be limited to NAME_MAX (plus NUL terminator). But I'd be a little more comfortable if we didn't use strcpy at all -- strlcpy is probably the right choice here. Might prefer to set buf->ie_event.len to MIN(strlen(name) + 1, sizeof(buf->ie_name)) too, so that non-DIAGNOSTIC builds will truncate the data rather than overrun the buffer.
Re: CVS commit: src/sys/compat/linux/common
On 2023-08-25 13:13, Taylor R Campbell wrote:
>> Module Name:src
>> Committed By: christos
>> Date: Wed Aug 23 19:17:59 UTC 2023
>>
>> Modified Files:
>> src/sys/compat/linux/common: linux_inotify.c
>>
>> Log Message:
>> put variable length structure at the end, so that clang does not complain.
>>
>> struct inotify_entry {
>> TAILQ_ENTRY(inotify_entry) ie_entries;
>> + charie_name[NAME_MAX + 1];
>> struct linux_inotify_event ie_event;
>> - charie_name[NAME_MAX+1];
>> };
>
> This can't be right, and it's a little unsettling that the problem
> isn't caught by any automatic tests.
>
> As I understand it, the `ie_name' member is supposed to provide
> storage for the `ie_event.name' array.
>
> So this looks like this change is going to lead either to a buffer
> overrun -- reading someone else's heap memory, or scribbling all over
> someone else's heap memory -- or to uninitialized or zero-filled
> memory being published to userland where there should be a pathname.
I don't think so. Notice that in inotify_read() two calls to uiomove()
are made, one on ie_event and one on ie_name... and in general this code
never accesses ie_event.name directly.
The reason I separated the fields in the first place was to make
allocation/deallocation simpler, not to use ie_name as storage for
ie_event.name.
Theo(dore)
Re: CVS commit: src/sys/compat/linux/common
> Module Name:src > Committed By: christos > Date: Thu Aug 24 19:51:24 UTC 2023 > > Modified Files: > src/sys/compat/linux/common: linux_inotify.c > > Log Message: > fix a locking bug (Theodore Preduta) > > if (needs_lock) > vn_lock(vp, LK_SHARED | LK_RETRY); > + else > + /* > +* XXX We need to temprarily drop v_interlock because > +* it may be temporarily acquired by biowait(). > +*/ > + mutex_exit(vp->v_interlock); > + KASSERT(!mutex_owned(vp->v_interlock)); > error = VOP_READDIR(vp, , fp->f_cred, , NULL, NULL); > if (needs_lock) > VOP_UNLOCK(vp); > + else > + mutex_enter(vp->v_interlock); This looks questionable. 1. Why is v_interlock held in the first place? 2. Why is it safe to release v_interlock here, if the caller holds it? 3. What is the lock order for all the locks involved? I see that inotify_readdir via get_inotify_dir_entries has two call sites, leading to the following lock orders: - linux_sys_inotify_add_watch -> mutex_enter(>ifd_lock) -> get_inotify_dir_entries(..., needs_lock=true) -> inotify_readdir(..., needs_lock=true) -> vn_lock(vp) -> VOP_READDIR(vp) - handle_write -> mutex_enter(>ifd_lock) -> get_inotify_dir_entries(..., needs_lock=false) -> inotify_readdir(..., needs_lock=true) -> mutex_exit(vp->v_interlock) -> VOP_READDIR(vp) Since VOP_READDIR requires vp to be locked, I can infer that the handle_write caller must already hold vp locked. But that means that we have ifd_lock -> vnode lock in one path, and vnode lock -> ifd_lock in another path, which is forbidden (unless there's some reason we can prove these paths never happen concurrently). I'm also suspicious of logic like this: mutex_enter(>f_lock); uio.uio_offset = fp->f_offset; mutex_exit(>f_lock); ... mutex_enter(>f_lock); fp->f_offset = uio.uio_offset; mutex_exit(>f_lock); If fp->f_offset can change concurrently while f_lock is released, this looks like a problem. But if it can't, why do we need the lock at all? I suspect this really does need a lock over the whole transaction (maybe fp->f_lock, maybe something else), which is not released while I/O is happening -- possibly not with mutex(9) but something interruptible instead.
Re: CVS commit: src/sys/compat/linux/common
> Module Name:src
> Committed By: christos
> Date: Wed Aug 23 19:17:59 UTC 2023
>
> Modified Files:
> src/sys/compat/linux/common: linux_inotify.c
>
> Log Message:
> put variable length structure at the end, so that clang does not complain.
>
> struct inotify_entry {
> TAILQ_ENTRY(inotify_entry) ie_entries;
> + charie_name[NAME_MAX + 1];
> struct linux_inotify_event ie_event;
> - charie_name[NAME_MAX+1];
> };
This can't be right, and it's a little unsettling that the problem
isn't caught by any automatic tests.
As I understand it, the `ie_name' member is supposed to provide
storage for the `ie_event.name' array.
So this looks like this change is going to lead either to a buffer
overrun -- reading someone else's heap memory, or scribbling all over
someone else's heap memory -- or to uninitialized or zero-filled
memory being published to userland where there should be a pathname.
We need to do this in a different way. (It's annoying that C doesn't
formally allow the original code, since in principle it is sensible.)
Perhaps we can delete the struct inotify_entry::ie_name member, and
instead of using
struct inotify_event *ie = kmem_zalloc(sizeof(*ie), ...);
do
struct inotify_event *ie = kmem_zalloc(
offsetof(struct inotify_event, ie_event.name[NAME_MAX + 1]),
...);
and similarly for kmem_free.
Re: CVS commit: src/sys/compat/linux
Yes, I committed it (I had the change in my tree).
Best,
christos
> On Aug 21, 2023, at 3:40 PM, Ryo ONODERA wrote:
>
> Hi,
>
> "Christos Zoulas" writes:
>
>> Module Name: src
>> Committed By:christos
>> Date:Sun Aug 20 18:08:57 UTC 2023
>>
>> Modified Files:
>> src/sys/compat/linux: files.linux
>>
>> Log Message:
>> add inotify (forgot to commit this)
>>
>>
>> To generate a diff of this commit:
>> cvs rdiff -u -r1.22 -r1.23 src/sys/compat/linux/files.linux
>>
>> Please note that diffs are not public domain; they are subject to the
>> copyright notices on the relevant files.
>
> The compat_linux kernel module is also required to have inotify.
> The following patch fixes my error from 'modload compat_linux'.
>
> # modload compat_linux
> [ 14619.3999280] kobj_checksyms, 1013: [compat_linux]: linker error: symbol
> `linux_sys_inotify_add_watch' not found
> [ 14619.3999280] kobj_checksyms, 1013: [compat_linux]: linker error: symbol
> `linux_sys_inotify_init1' not found
> [ 14619.3999280] kobj_checksyms, 1013: [compat_linux]: linker error: symbol
> `linux_sys_inotify_rm_watch' not found
> [ 14619.3999280] kobj_checksyms, 1013: [compat_linux]: linker error: symbol
> `linux_inotify_init' not found
> [ 14619.3999280] kobj_checksyms, 1013: [compat_linux]: linker error: symbol
> `linux_sys_inotify_init' not found
> [ 14619.3999280] kobj_checksyms, 1013: [compat_linux]: linker error: symbol
> `linux_inotify_fini' not found
> [ 14619.3999280] WARNING: module error: unable to affix module
> `compat_linux', error 8
>
> Patch:
> Index: sys/modules/compat_linux/Makefile
> ===
> RCS file: /cvsroot/src/sys/modules/compat_linux/Makefile,v
> retrieving revision 1.6
> diff -u -r1.6 Makefile
> --- sys/modules/compat_linux/Makefile 9 Oct 2021 07:01:35 - 1.6
> +++ sys/modules/compat_linux/Makefile 21 Aug 2023 19:37:08 -
> @@ -9,7 +9,8 @@
>
> .PATH:${S}/compat/linux/common
> SRCS= linux_blkio.c linux_cdrom.c linux_errno.c linux_exec.c
> -SRCS+= linux_fdio.c linux_file.c linux_hdio.c linux_ioctl.c
> +SRCS+= linux_fdio.c linux_file.c linux_hdio.c linux_inotify.c
> +SRCS+= linux_ioctl.c
> SRCS+=linux_ipc.c linux_misc.c linux_mtio.c linux_sched.c
> SRCS+=linux_sg.c linux_signal.c linux_signo.c linux_socket.c
> SRCS+=linux_sysctl.c linux_termios.c linux_time.c linux_mod.c
>
> Thank you.
>
> --
> Ryo ONODERA // r...@tetera.org
> PGP fingerprint = 82A2 DC91 76E0 A10A 8ABB FD1B F404 27FA C7D1 15F3
signature.asc
Description: Message signed with OpenPGP
Re: CVS commit: src/sys/compat/linux
Hi,
"Christos Zoulas" writes:
> Module Name: src
> Committed By: christos
> Date: Sun Aug 20 18:08:57 UTC 2023
>
> Modified Files:
> src/sys/compat/linux: files.linux
>
> Log Message:
> add inotify (forgot to commit this)
>
>
> To generate a diff of this commit:
> cvs rdiff -u -r1.22 -r1.23 src/sys/compat/linux/files.linux
>
> Please note that diffs are not public domain; they are subject to the
> copyright notices on the relevant files.
The compat_linux kernel module is also required to have inotify.
The following patch fixes my error from 'modload compat_linux'.
# modload compat_linux
[ 14619.3999280] kobj_checksyms, 1013: [compat_linux]: linker error: symbol
`linux_sys_inotify_add_watch' not found
[ 14619.3999280] kobj_checksyms, 1013: [compat_linux]: linker error: symbol
`linux_sys_inotify_init1' not found
[ 14619.3999280] kobj_checksyms, 1013: [compat_linux]: linker error: symbol
`linux_sys_inotify_rm_watch' not found
[ 14619.3999280] kobj_checksyms, 1013: [compat_linux]: linker error: symbol
`linux_inotify_init' not found
[ 14619.3999280] kobj_checksyms, 1013: [compat_linux]: linker error: symbol
`linux_sys_inotify_init' not found
[ 14619.3999280] kobj_checksyms, 1013: [compat_linux]: linker error: symbol
`linux_inotify_fini' not found
[ 14619.3999280] WARNING: module error: unable to affix module `compat_linux',
error 8
Patch:
Index: sys/modules/compat_linux/Makefile
===
RCS file: /cvsroot/src/sys/modules/compat_linux/Makefile,v
retrieving revision 1.6
diff -u -r1.6 Makefile
--- sys/modules/compat_linux/Makefile 9 Oct 2021 07:01:35 - 1.6
+++ sys/modules/compat_linux/Makefile 21 Aug 2023 19:37:08 -
@@ -9,7 +9,8 @@
.PATH: ${S}/compat/linux/common
SRCS= linux_blkio.c linux_cdrom.c linux_errno.c linux_exec.c
-SRCS+= linux_fdio.c linux_file.c linux_hdio.c linux_ioctl.c
+SRCS+= linux_fdio.c linux_file.c linux_hdio.c linux_inotify.c
+SRCS+= linux_ioctl.c
SRCS+= linux_ipc.c linux_misc.c linux_mtio.c linux_sched.c
SRCS+= linux_sg.c linux_signal.c linux_signo.c linux_socket.c
SRCS+= linux_sysctl.c linux_termios.c linux_time.c linux_mod.c
Thank you.
--
Ryo ONODERA // r...@tetera.org
PGP fingerprint = 82A2 DC91 76E0 A10A 8ABB FD1B F404 27FA C7D1 15F3
Re: CVS commit: src/external/gpl3/gdb/dist/gnulib
Ah, it must be better. Thank you for useful comment as always :) I've confirmed the same files are generated. I will commit soon, but commit mail seems to be delayed for some reasons... Thanks, rin On Sun, Aug 20, 2023 at 11:17 AM Valery Ushakov wrote: > > On Sun, Aug 20, 2023 at 02:02:40 +, Rin Okuyama wrote: > > > Modified Files: > > src/external/gpl3/gdb/dist/gnulib: configure > > src/external/gpl3/gdb/dist/gnulib/import/m4: rename.m4 > > > > Log Message: > > gdb/gnulib: ``guess yes'' to rename(2) checks for NetBSD > > Can't we just set gl_cv_func_rename_slash_dst_works in the environment > when running configure. I'm pretty sure we are already doing that > somewhere to force some misdetected features, and that's in our > makefiles, not in the imported code, so it's unlikely to get lost in a > merge on new imports later. > > -uwe >
Re: CVS commit: src/external/gpl3/gdb/dist/gnulib
On Sun, Aug 20, 2023 at 02:02:40 +, Rin Okuyama wrote: > Modified Files: > src/external/gpl3/gdb/dist/gnulib: configure > src/external/gpl3/gdb/dist/gnulib/import/m4: rename.m4 > > Log Message: > gdb/gnulib: ``guess yes'' to rename(2) checks for NetBSD Can't we just set gl_cv_func_rename_slash_dst_works in the environment when running configure. I'm pretty sure we are already doing that somewhere to force some misdetected features, and that's in our makefiles, not in the imported code, so it's unlikely to get lost in a merge on new imports later. -uwe
Re: CVS commit: src/external/gpl3/gdb/dist/gdb
> For vax, something still wrong. This was due to my test environment built by GCC 12. For system built by GCC 10, GDB 13 works just fine (except for C++ exception handling), as far as I can see. Thanks, rin On Thu, Aug 17, 2023 at 4:45 PM Rin Okuyama wrote: > > Module Name:src > Committed By: rin > Date: Thu Aug 17 07:45:11 UTC 2023 > > Modified Files: > src/external/gpl3/gdb/dist/gdb: configure.tgt > > Log Message: > gdb/vax and ia64: Add missing tdep files for NetBSD > > They are also missing for gdb 11. > > XXX > For vax, something still wrong. For ia64, compile test only. > > > To generate a diff of this commit: > cvs rdiff -u -r1.30 -r1.31 src/external/gpl3/gdb/dist/gdb/configure.tgt > > Please note that diffs are not public domain; they are subject to the > copyright notices on the relevant files. >
Re: CVS commit: src/external/gpl3/gdb/dist/bfd
This was not correct. Core file support for NetBSD/riscv was present for our local version of GDB 11, and lost during merge. Thanks, rin On Thu, Aug 17, 2023 at 4:37 PM Rin Okuyama wrote: > > Module Name:src > Committed By: rin > Date: Thu Aug 17 07:37:36 UTC 2023 > > Modified Files: > src/external/gpl3/gdb/dist/bfd: configure > > Log Message: > gdb/bfd: Add initial support to NetBSD-style core file for riscv > > Not working yet: > > Reading symbols from cat... > Reading symbols from /usr/libdata/debug//bin/cat.debug... > > warning: Couldn't find general-purpose registers in core file. > Core was generated by `cat'. > Program terminated with signal SIGQUIT, Quit. > > warning: Couldn't find general-purpose registers in core file. > #0 in ?? () > (gdb) > > > To generate a diff of this commit: > cvs rdiff -u -r1.15 -r1.16 src/external/gpl3/gdb/dist/bfd/configure > > Please note that diffs are not public domain; they are subject to the > copyright notices on the relevant files. >
Re: CVS commit: src/external/gpl3/binutils.old/dist/bfd
> binutils/bfd: Correct auxv offset for NetBSD, from gdb/bfd binutils.old/bfd for this commit... rin On Thu, Aug 17, 2023 at 3:49 PM Rin Okuyama wrote: > > Module Name:src > Committed By: rin > Date: Thu Aug 17 06:49:01 UTC 2023 > > Modified Files: > src/external/gpl3/binutils.old/dist/bfd: elf.c > > Log Message: > binutils/bfd: Correct auxv offset for NetBSD, from gdb/bfd > > Fallout not observed yet as far as I can see although... > > > To generate a diff of this commit: > cvs rdiff -u -r1.10 -r1.11 src/external/gpl3/binutils.old/dist/bfd/elf.c > > Please note that diffs are not public domain; they are subject to the > copyright notices on the relevant files. >
Re: CVS commit: src/external/gpl3/gdb/dist/gdb
> gcc/ppc: Register NetBSD OSABI for rs6000, lost during merge oops, apparently "gdb/ppc", not "gcc". Thanks, rin On Thu, Aug 17, 2023 at 2:53 PM Rin Okuyama wrote: > > Module Name:src > Committed By: rin > Date: Thu Aug 17 05:53:45 UTC 2023 > > Modified Files: > src/external/gpl3/gdb/dist/gdb: ppc-netbsd-tdep.c > > Log Message: > gcc/ppc: Register NetBSD OSABI for rs6000, lost during merge > > Otherwise, OSABI is undetermined until executable is loaded. > > Add comments also. > > > To generate a diff of this commit: > cvs rdiff -u -r1.2 -r1.3 src/external/gpl3/gdb/dist/gdb/ppc-netbsd-tdep.c > > Please note that diffs are not public domain; they are subject to the > copyright notices on the relevant files. >
Re: CVS commit: src
I would agree with Taylor, it is not tested well enough to be enabled by default and mainly unmaintained by now. I guess the better solution would be to attempt to integrate openchrome-drm instead at some point in the future. I think making viadrmums as kernel module is quite a good trade off right now for those who need/want to use it and it saves from kernel compilation. Will do pull-up to netbsd-10 soon for this change. I may try to do some additional testing to verify how much it is utilized right now and were performance makes a difference (and likely would need to investigate what is wrong with VX900). I believe 3D acceleration is unsupported. On Sun, Aug 13, 2023 at 8:05 PM Taylor R Campbell wrote: > > > Date: Thu, 10 Aug 2023 07:42:53 +1000 > > from: matthew green > > > > > Log Message: > > > viadrmums(4): build legacy VIA DRM UMS driver module for amd64. > > > > > > This driver is not built-in by default, thus loadable module can help > > > (un)lucky > > > > if it works, why isn't it in GENERIC as well as a module? > > Couple reasons: > > 1. I never adequately tested it. I started X a few years ago (and a >couple drm updates ago) but that's it. (Also only on 32-bit VIA.) >Maybe andvar can test it more extensively now, though -- glxgears, >x11perf, firefox, > >Even better if we can get a sampling of code coverage, e.g. maybe >with dtrace to count function calls at least, to confirm it's >actually getting exercised. > > 2. It's a legacy UMS driver, which is inherently sketchy (display >configuration requires granting userland direct access to pci and >device registers) and increasingly unmaintained even upstream. > >So I put in some infrastructure to allow UMS drivers to be loaded >dynamically, so it wouldn't be necessary to have sketchy business >like that in the kernel by default. > >I kind of intended to give the same treatment to the other legacy >UMS drivers like mga (matrox), r128, sis, tdfx, , but it's been >a very low priority so I haven't gotten a round tuit (and I only >have hardware for mga and that's in a build server I don't like to >mess with).
Re: CVS commit: src
> Date: Thu, 10 Aug 2023 07:42:53 +1000 > from: matthew green > > > Log Message: > > viadrmums(4): build legacy VIA DRM UMS driver module for amd64. > > > > This driver is not built-in by default, thus loadable module can help > > (un)lucky > > if it works, why isn't it in GENERIC as well as a module? Couple reasons: 1. I never adequately tested it. I started X a few years ago (and a couple drm updates ago) but that's it. (Also only on 32-bit VIA.) Maybe andvar can test it more extensively now, though -- glxgears, x11perf, firefox, Even better if we can get a sampling of code coverage, e.g. maybe with dtrace to count function calls at least, to confirm it's actually getting exercised. 2. It's a legacy UMS driver, which is inherently sketchy (display configuration requires granting userland direct access to pci and device registers) and increasingly unmaintained even upstream. So I put in some infrastructure to allow UMS drivers to be loaded dynamically, so it wouldn't be necessary to have sketchy business like that in the kernel by default. I kind of intended to give the same treatment to the other legacy UMS drivers like mga (matrox), r128, sis, tdfx, , but it's been a very low priority so I haven't gotten a round tuit (and I only have hardware for mga and that's in a build server I don't like to mess with).
Re: CVS commit: src
On Sat, 12 Aug 2023 at 16:16, Andrius V wrote: > > Hi, > > Sorry, didn't notice the email until today. To answer your question > why it is not enabled in GENERIC by default, I am not sure > (Taylor(?)), it was commented since the day one both on i386 and > amd64. > > However, I have some doubts if it is very relevant driver to enable > it. From the context perspective, viadrmums is also a legacy driver, > Linux have already removed it in the latest kernel source code, and > new https://cgit.freedesktop.org/openchrome/drm-openchrome/ was > developed, but haven't made it to official kernel yet (and hard to > tell when and if it will happen at all)). VIA integrated graphics > drivers development was a always a bit of a sad story: lack of > interest, manpower/skills, lack of VIA participation, the hardware is > pretty obscure and under-performing, and likely more suitable for non > graphics related tasks. Myself I used it mainly for NAS and testing > platform, thus I need only basic graphics support. Nevertheless, if it > doesn't affect the development much and it does seem to be work in the > machines I own, I can enable it. Some of us are all about the legacy hardware :) Seriously, one of the areas in which NetBSD's reputation is relatively better is in older x86 hardware, so if enabling it is likely to give a better experience for some set of hardware while being very unlikely to make things worse, I'd support enabling it David
Re: CVS commit: src
Hi, Sorry, didn't notice the email until today. To answer your question why it is not enabled in GENERIC by default, I am not sure (Taylor(?)), it was commented since the day one both on i386 and amd64. However, I have some doubts if it is very relevant driver to enable it. From the context perspective, viadrmums is also a legacy driver, Linux have already removed it in the latest kernel source code, and new https://cgit.freedesktop.org/openchrome/drm-openchrome/ was developed, but haven't made it to official kernel yet (and hard to tell when and if it will happen at all)). VIA integrated graphics drivers development was a always a bit of a sad story: lack of interest, manpower/skills, lack of VIA participation, the hardware is pretty obscure and under-performing, and likely more suitable for non graphics related tasks. Myself I used it mainly for NAS and testing platform, thus I need only basic graphics support. Nevertheless, if it doesn't affect the development much and it does seem to be work in the machines I own, I can enable it. That also reminds me that I wanted to finish unichromefb driver in our code base, which is unfinished and limited to VX900 support now if I am not mistaken... Maybe it is time to spend some effort on that... Regards, Andrius V On Thu, Aug 10, 2023 at 12:42 AM matthew green wrote: > > > Log Message: > > viadrmums(4): build legacy VIA DRM UMS driver module for amd64. > > > > This driver is not built-in by default, thus loadable module can help > > (un)lucky > > if it works, why isn't it in GENERIC as well as a module? > > thanks. > > > .mrg.
Re: CVS commit: src
In article <20230803122447.d5e18f...@cvs.netbsd.org>, Nia Alarie wrote: >-=-=-=-=-=- > >Module Name: src >Committed By: nia >Date: Thu Aug 3 12:24:47 UTC 2023 > >Modified Files: > src/distrib/sets/lists/comp: mi > src/distrib/sets/lists/debug: mi > src/distrib/sets/lists/tests: mi > src/lib/libc/sys: Makefile.inc > src/sys/sys: Makefile > src/tests/kernel: Makefile > >Log Message: >Revert addition of epoll to libc until discussion concludes Where was this discussed? christos
Re: CVS commit: src/sys/dev/pci
> Date: Thu, 10 Aug 2023 17:42:35 +0900 > From: Kengo NAKAHARA > > Could you tell me how you test this fix for future reference? I asked skrll@ to boot a VM with vmxnet3 and hammer on it with a combination of: 1. dhcp 2. host$ nc -l 54321 /dev/null guest$ nc host 54321 /dev/null 3. for i in `jot 4`; do jot 100 | while read i; do ifconfig vmxnet0 down ifconfig vmxnet0 up done & done wait and then see if the network continued functioning. However, I didn't do anything to verify that we could trigger all the problems that I noticed by code inspection. I didn't do that mostly because I'd been sitting on this patch for a year, and I didn't want it to languish indefinitely with obvious bugs lurking in vmxnet(4). Here are a couple more things that it would be good to do: 1. Create a sysctl knob to simulate watchdog failure and trigger reset. I don't think we tested this path at all, but it's a common bug with an easy fix -- defer to workqueue. Some other drivers have a sysctl knob like this: wm(4), bge(4). 2. Verify that SIOCADDMULTI and SIOCDELMULTI don't deadlock when run concurrently with ifconfig up/down. vmxnet(4) still has a serious bug: the `core lock' can still lead to a deadlock with reset and softint. It should be removed from most uses, and be limited to cover only mii transactions like I did for usbnet(4) last year.
Re: CVS commit: src/sys/dev/pci
Hi, On 2023/08/10 18:07, Nick Hudson wrote: On 10/08/2023 09:42, Kengo NAKAHARA wrote: Hi, Could you tell me how you test this fix for future reference? He didn't - I did. :) Taylor suggested running with network traffic and doing ifconfig down/up. To generate network traffic Taylor suggested host$ nc -l 54321 /dev/null guest$ nc host 54321 /dev/null and I did for i in `jot 64`; do ifconfig vmx0 down sleep 1 ifconfig vmx0 up done Nick I see. I want to know the traffic generator and ioctl jobs, that is exactly what is. Thank you for your comment. Thanks, -- // Internet Initiative Japan Inc. Device Engineering Section, Core Product Development Department, Network Division, Technology Unit Kengo NAKAHARA
Re: CVS commit: src/sys/dev/pci
On 10/08/2023 09:42, Kengo NAKAHARA wrote: Hi, Could you tell me how you test this fix for future reference? He didn't - I did. :) Taylor suggested running with network traffic and doing ifconfig down/up. To generate network traffic Taylor suggested host$ nc -l 54321 /dev/null guest$ nc host 54321 /dev/null and I did for i in `jot 64`; do ifconfig vmx0 down sleep 1 ifconfig vmx0 up done Nick
Re: CVS commit: src/sys/dev/pci
Hi, Could you tell me how you test this fix for future reference? Thanks, On 2023/08/10 17:24, Taylor R Campbell wrote: Module Name:src Committed By: riastradh Date: Thu Aug 10 08:24:45 UTC 2023 Modified Files: src/sys/dev/pci: if_vmx.c Log Message: vmxnet(4): Fix various MP bugs. - Defer reset to workqueue. => vmxnet3_stop_locked is forbidden in softint. => XXX Problem: We still take the core lock in softint, and we still take the core lock around vmxnet3_stop_locked. TBD. - Touch if_flags only under IFNET_LOCK. => Cache ifp->if_flags & IFF_PROMISC in vmxnet3_ifflags_cb. => Don't call vmxnet3_set_rxfilter unless up and running; cache this as vmx_mcastactive. Use ENETRESET in vmxnet3_ifflags_cb instead of calling vmxnet3_set_rxfilter directly. . (The cache is currently serialized by the core lock, but it might reasonably be serialized by an independent lock like in usbnet(9).) - Fix vmxnet3_stop_rendezvous so it actually does something. => New vxtxq_stopping, vxrxq_stopping variables synchronize with Rx/Tx interrupt handlers. - Sprinkle IFNET_LOCK and core lock assertions. To generate a diff of this commit: cvs rdiff -u -r1.11 -r1.12 src/sys/dev/pci/if_vmx.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. -- // Internet Initiative Japan Inc. Device Engineering Section, Core Product Development Department, Network Division, Technology Unit Kengo NAKAHARA
re: CVS commit: src
> Log Message: > viadrmums(4): build legacy VIA DRM UMS driver module for amd64. > > This driver is not built-in by default, thus loadable module can help > (un)lucky if it works, why isn't it in GENERIC as well as a module? thanks. .mrg.
re: CVS commit: src/share/mk
please review this. i'll try to figure out tests for everything, though it seems annoying :) https://www.netbsd.org/~mrg/gcc12-use-after-free.diff it should handle all the open use-after-free problems. .mrg. ps: you'll notice no new headers needed for ptrdiff_t usage ;)
re: CVS commit: src/share/mk
matthew green writes: > > > - used = dst - conv->wbuff; > > > + size_t sused = (uintptr_t)dst - (uintptr_t)conv->wbuff; > > > > Any particular reason why there is a cast to uintptr_t here? I don't > > think there is a guarantee that you can calculate an offset by > > subtracting uintptr_ts calculated from pointers. The description in the > > C Standard only guarantees that you can convert them back to a pointer > > which compares the same to the original, but that's it. I don't find any > > other promises about uintptr_t. > > in this case, they're not necessary it seems. probably left > over from my initial attempts at this workaround. uh, apparently i forgot to save the file before compiling, because simply removing them returns the sign-compare warning, but that's fixable by using ptrdiff_t. .mrg.
re: CVS commit: src/share/mk
> > - used = dst - conv->wbuff; > > + size_t sused = (uintptr_t)dst - (uintptr_t)conv->wbuff; > > Any particular reason why there is a cast to uintptr_t here? I don't > think there is a guarantee that you can calculate an offset by > subtracting uintptr_ts calculated from pointers. The description in the > C Standard only guarantees that you can convert them back to a pointer > which compares the same to the original, but that's it. I don't find any > other promises about uintptr_t. in this case, they're not necessary it seems. probably left over from my initial attempts at this workaround. .mrg.
Re: CVS commit: src/share/mk
On Tue 08 Aug 2023 at 14:10:41 +0200, Joerg Sonnenberger wrote: > On Tue, Aug 08, 2023 at 01:42:39PM +0200, Rhialto wrote: > > On Tue 08 Aug 2023 at 09:44:41 +1000, matthew green wrote: > > > Index: lib/libedit/chartype.c > > > === > > > RCS file: /cvsroot/src/lib/libedit/chartype.c,v > > > retrieving revision 1.36 > > > diff -p -u -r1.36 chartype.c > > > --- lib/libedit/chartype.c30 Oct 2022 19:11:31 - 1.36 > > > +++ lib/libedit/chartype.c7 Aug 2023 23:41:44 - > > > @@ -235,17 +235,17 @@ ct_visual_string(const wchar_t *s, ct_bu > > > } > > > > > > /* failed to encode, need more buffer space */ > > > - used = dst - conv->wbuff; > > > + size_t sused = (uintptr_t)dst - (uintptr_t)conv->wbuff; > > > > Any particular reason why there is a cast to uintptr_t here? I don't > > think there is a guarantee that you can calculate an offset by > > subtracting uintptr_ts calculated from pointers. The description in the > > C Standard only guarantees that you can convert them back to a pointer > > which compares the same to the original, but that's it. I don't find any > > other promises about uintptr_t. > > Given that we used to make this assumption for offsetof like most > systems, this seems to be portable naval gazing to me. It is one thing to hide such an assumption away in a macro (and with all compilers currently in use, offsetof() is mapped to __builtin_offsetof() (see ), which quite likely exists because of the unstandardness of the other version), but quite another to open-code it again and again in general code. Think of the PDP-10 port! I was expecting some sort of answer related to unsigned vs signed sizes and differences, or something like that, for which there is likely a cleaner solution. > Joerg -Olaf. -- ___ Olaf 'Rhialto' Seibert \X/ There is no AI. There is just someone else's work. --I. Rose signature.asc Description: PGP signature
Re: CVS commit: src/share/mk
On Tue, Aug 08, 2023 at 01:42:39PM +0200, Rhialto wrote: > On Tue 08 Aug 2023 at 09:44:41 +1000, matthew green wrote: > > Index: lib/libedit/chartype.c > > === > > RCS file: /cvsroot/src/lib/libedit/chartype.c,v > > retrieving revision 1.36 > > diff -p -u -r1.36 chartype.c > > --- lib/libedit/chartype.c 30 Oct 2022 19:11:31 - 1.36 > > +++ lib/libedit/chartype.c 7 Aug 2023 23:41:44 - > > @@ -235,17 +235,17 @@ ct_visual_string(const wchar_t *s, ct_bu > > } > > > > /* failed to encode, need more buffer space */ > > - used = dst - conv->wbuff; > > + size_t sused = (uintptr_t)dst - (uintptr_t)conv->wbuff; > > Any particular reason why there is a cast to uintptr_t here? I don't > think there is a guarantee that you can calculate an offset by > subtracting uintptr_ts calculated from pointers. The description in the > C Standard only guarantees that you can convert them back to a pointer > which compares the same to the original, but that's it. I don't find any > other promises about uintptr_t. Given that we used to make this assumption for offsetof like most systems, this seems to be portable naval gazing to me. Joerg
Re: CVS commit: src/share/mk
On Tue 08 Aug 2023 at 09:44:41 +1000, matthew green wrote: > Index: lib/libedit/chartype.c > === > RCS file: /cvsroot/src/lib/libedit/chartype.c,v > retrieving revision 1.36 > diff -p -u -r1.36 chartype.c > --- lib/libedit/chartype.c30 Oct 2022 19:11:31 - 1.36 > +++ lib/libedit/chartype.c7 Aug 2023 23:41:44 - > @@ -235,17 +235,17 @@ ct_visual_string(const wchar_t *s, ct_bu > } > > /* failed to encode, need more buffer space */ > - used = dst - conv->wbuff; > + size_t sused = (uintptr_t)dst - (uintptr_t)conv->wbuff; Any particular reason why there is a cast to uintptr_t here? I don't think there is a guarantee that you can calculate an offset by subtracting uintptr_ts calculated from pointers. The description in the C Standard only guarantees that you can convert them back to a pointer which compares the same to the original, but that's it. I don't find any other promises about uintptr_t. -Olaf. -- ___ Olaf 'Rhialto' Seibert \X/ There is no AI. There is just someone else's work. --I. Rose signature.asc Description: PGP signature
Re: CVS commit: src/sys/arch
On Tue, Aug 08, 2023 at 04:01:19PM +1000, matthew green wrote: > Joerg Sonnenberger writes: > > On Thu, Aug 03, 2023 at 08:16:31AM +, matthew green wrote: > > > Module Name: src > > > Committed By: mrg > > > Date: Thu Aug 3 08:16:31 UTC 2023 > > > > > > Modified Files: > > > src/sys/arch/evbarm/gumstix: gumstix_machdep.c > > > src/sys/arch/evbarm/ixm1200: ixm1200_machdep.c > > > src/sys/arch/hpcarm/hpcarm: pxa2x0_hpc_machdep.c sa11x0_hpc_machdep.c > > > src/sys/arch/hppa/stand: Makefile.buildboot > > > src/sys/arch/m68k/m68k: regdump.c > > > src/sys/arch/macppc/macppc: cpu.c > > > > > > Log Message: > > > ignore "-Warray-bounds" for various low level platform code that knows > > > how something is setup but technically is undefined behaviour. the > > > most common here is "extern int end;" and then using offsets of "" > > > that are outside the bounds of this 4-byte integer. > > > > > > these uses are almost certainly all OK in reality. > > > > Are you sure that GCC's optimizer is not going to break any of those > > assumptions? We had to go through quite some trouble in crtbegin.c for > > similar patterns? > > nope, infact i'm not. i'll revert these. > > do you have good ways to fix these from that crt issue? Not really, I just remember that we needed a few iterations. The last round was especially mysterious as weak references suddenly couldn't alias extern symbol. Joerg
Re: CVS commit: src/share/mk
> Date: Mon, 7 Aug 2023 23:58:50 +0200 > From: Tobias Nygren > > Is this sort of fix acceptable for the above cases? > > + ptrdiff_t offset = pos - buf; > new_buf = realloc(buf, buf_size); > if (!new_buf) > err(2, "realloc of linebuf to %zu bytes failed", > buf_size); > - > + > end = new_buf + buf_size; > - pos = new_buf + (pos - buf); > + pos = new_buf + offset; > buf = new_buf; Yes, this is a good approach. Even if it's suboptimal in some cases, it is very easy to audit mechanical changes, which is important if there are a lot of them. Any further case-specific simplifications (like changing ptrdiff_t to size_t, since it will always be nonnegative here; just using `size_t offset = buf_size' before `buf_size *= 2', since since pos == end and end == buf + buf_size) can be done afterward in a separate commit.
re: CVS commit: src/sys/arch
Joerg Sonnenberger writes: > On Thu, Aug 03, 2023 at 08:16:31AM +, matthew green wrote: > > Module Name:src > > Committed By: mrg > > Date: Thu Aug 3 08:16:31 UTC 2023 > > > > Modified Files: > > src/sys/arch/evbarm/gumstix: gumstix_machdep.c > > src/sys/arch/evbarm/ixm1200: ixm1200_machdep.c > > src/sys/arch/hpcarm/hpcarm: pxa2x0_hpc_machdep.c sa11x0_hpc_machdep.c > > src/sys/arch/hppa/stand: Makefile.buildboot > > src/sys/arch/m68k/m68k: regdump.c > > src/sys/arch/macppc/macppc: cpu.c > > > > Log Message: > > ignore "-Warray-bounds" for various low level platform code that knows > > how something is setup but technically is undefined behaviour. the > > most common here is "extern int end;" and then using offsets of "" > > that are outside the bounds of this 4-byte integer. > > > > these uses are almost certainly all OK in reality. > > Are you sure that GCC's optimizer is not going to break any of those > assumptions? We had to go through quite some trouble in crtbegin.c for > similar patterns? nope, infact i'm not. i'll revert these. do you have good ways to fix these from that crt issue? thanks. .mrg.
re: CVS commit: src/share/mk
Valery Ushakov writes:
> On Mon, Aug 07, 2023 at 23:58:50 +0200, Tobias Nygren wrote:
>
> > Is this sort of fix acceptable for the above cases?
> [...]
> > + ptrdiff_t offset = pos - buf;
> [...]
> > - pos = new_buf + (pos - buf);
> > + pos = new_buf + offset;
>
> I think so. But e.g. in this particular case I don't see why pos
> pointer is needed at all. If pos is made into a size_t pos; index
> into the buf instead of a separate pointer, one avoids the whole thing
> and "end" can be g/c'ed too, b/c you can just compare the index to the
> "buf_size". But the above kind of fix has the advantage of being more
> or less mechanical.
yup, i agree. if we can fix it better, please do, but i'm all for
making it "less bad" if not properly fixed like the above.
i've used this idiom in a couple of places, but didn't quite get it
working in several others.
these are the problematic files i've see:
lib/libc/net/gethnamaddr.c
lib/libedit/chartype.c
lib/libkvm/kvm_proc.c
usr.bin/find/misc.c
usr.bin/mail/fio.c
external/bsd/pdisk/dist/io.c
usr.bin/rs/rs.c
usr.bin/sort/files.c
the first 3 i worked around, eg below, but i think i only tested
the gethnamaddr.c portion so far.
.mrg.
Index: lib/libc/net/gethnamaddr.c
===
RCS file: /cvsroot/src/lib/libc/net/gethnamaddr.c,v
retrieving revision 1.94
diff -p -u -r1.94 gethnamaddr.c
--- lib/libc/net/gethnamaddr.c 19 Apr 2022 20:32:15 - 1.94
+++ lib/libc/net/gethnamaddr.c 7 Aug 2023 23:41:44 -
@@ -110,10 +110,11 @@ __weak_alias(gethostent,_gethostent)
#define addalias(d, s, arr, siz) do { \
if (d >= [siz]) { \
+ size_t _off = d - arr; \
char **xptr = realloc(arr, (siz + 10) * sizeof(*arr)); \
if (xptr == NULL) \
goto nospc; \
- d = xptr + (d - arr); \
+ d = xptr + _off;\
arr = xptr; \
siz += 10; \
} \
Index: lib/libedit/chartype.c
===
RCS file: /cvsroot/src/lib/libedit/chartype.c,v
retrieving revision 1.36
diff -p -u -r1.36 chartype.c
--- lib/libedit/chartype.c 30 Oct 2022 19:11:31 - 1.36
+++ lib/libedit/chartype.c 7 Aug 2023 23:41:44 -
@@ -235,17 +235,17 @@ ct_visual_string(const wchar_t *s, ct_bu
}
/* failed to encode, need more buffer space */
- used = dst - conv->wbuff;
+ size_t sused = (uintptr_t)dst - (uintptr_t)conv->wbuff;
if (ct_conv_wbuff_resize(conv, conv->wsize + CT_BUFSIZ) == -1)
return NULL;
- dst = conv->wbuff + used;
+ dst = conv->wbuff + sused;
}
if (dst >= (conv->wbuff + conv->wsize)) { /* sigh */
- used = dst - conv->wbuff;
+ size_t sused = (uintptr_t)dst - (uintptr_t)conv->wbuff;
if (ct_conv_wbuff_resize(conv, conv->wsize + CT_BUFSIZ) == -1)
return NULL;
- dst = conv->wbuff + used;
+ dst = conv->wbuff + sused;
}
*dst = L'\0';
Index: lib/libkvm/kvm_proc.c
===
RCS file: /cvsroot/src/lib/libkvm/kvm_proc.c,v
retrieving revision 1.98
diff -p -u -r1.98 kvm_proc.c
--- lib/libkvm/kvm_proc.c 19 Apr 2022 20:32:16 - 1.98
+++ lib/libkvm/kvm_proc.c 7 Aug 2023 23:41:44 -
@@ -980,7 +980,7 @@ kvm_argv(kvm_t *kd, const struct minipro
if (len + cc > kd->argspc_len) {
ptrdiff_t off;
char **pp;
- char *op = kd->argspc;
+ uintptr_t op = (uintptr_t)kd->argspc;
kd->argspc_len *= 2;
kd->argspc = _kvm_realloc(kd, kd->argspc,
@@ -991,7 +991,7 @@ kvm_argv(kvm_t *kd, const struct minipro
* Adjust argv pointers in case realloc moved
* the string space.
*/
- off = kd->argspc - op;
+ off = (uintptr_t)kd->argspc - op;
for (pp = kd->argv; pp < argv; pp++)
*pp += off;
ap += off;
Re: Restoring pointers after realloc (was: Re: CVS commit: src/share/mk)
On Tue, Aug 08, 2023 at 00:44:41 +0200, Roland Illig wrote: > Am 07.08.2023 um 23:58 schrieb Tobias Nygren: > > Is this sort of fix acceptable for the above cases? > > > > + ptrdiff_t offset = pos - buf; > > new_buf = realloc(buf, buf_size); > > - pos = new_buf + (pos - buf); > > + pos = new_buf + offset; > > Yes. > > Instead of ptrdiff_t, I prefer to use size_t to avoid having to include > , even if that means an additional type cast at WARNS=6 level: > > > + size_t offset = (size_t)(pos - buf); I just had a massive cognitive dissonance moment... :) -uwe
Re: CVS commit: src/share/mk
On Mon, Aug 07, 2023 at 23:58:50 +0200, Tobias Nygren wrote: > Is this sort of fix acceptable for the above cases? [...] > + ptrdiff_t offset = pos - buf; [...] > - pos = new_buf + (pos - buf); > + pos = new_buf + offset; I think so. But e.g. in this particular case I don't see why pos pointer is needed at all. If pos is made into a size_t pos; index into the buf instead of a separate pointer, one avoids the whole thing and "end" can be g/c'ed too, b/c you can just compare the index to the "buf_size". But the above kind of fix has the advantage of being more or less mechanical. -uwe
Restoring pointers after realloc (was: Re: CVS commit: src/share/mk)
Am 07.08.2023 um 23:58 schrieb Tobias Nygren: > Is this sort of fix acceptable for the above cases? > > + ptrdiff_t offset = pos - buf; > new_buf = realloc(buf, buf_size); > - pos = new_buf + (pos - buf); > + pos = new_buf + offset; Yes. Instead of ptrdiff_t, I prefer to use size_t to avoid having to include , even if that means an additional type cast at WARNS=6 level: > + size_t offset = (size_t)(pos - buf); Roland
Re: CVS commit: src/share/mk
On Thu, 3 Aug 2023 23:30:31 +0900 Rin Okuyama wrote: > On 2023/08/03 23:23, Valery Ushakov wrote: > > On Thu, Aug 03, 2023 at 13:33:27 +, Rin Okuyama wrote: > > > >> -Wuse-after-free for GCC 12 is premature. It fires on a common idiom: > >> > >>newbuf = realloc(buf, size); > >>p = newbuf + (p - buf); > >> > >> Let shut this up for GCC 12 (with hoping it gets improved for 13!). > > > > C99 says > > > > J.2 Undefined behavior > > > > [#1] The behavior is undefined in the following > > circumstances: > > [...] > > -- The value of a pointer to an object whose lifetime has > > ended is used (6.2.4). > > > > > > Yes, for the "obvious" implementation of pointers as addresses the > > above idiom happens to work, but it doesn't make that idiom any less > > UB. > > Ah, I only thought about "obvious" impl. Thank you for kind > explanation! I will revert them for now. Hi, Is this sort of fix acceptable for the above cases? -Tobias RCS file: /cvsroot/src/usr.bin/sort/files.c,v retrieving revision 1.42 diff -p -u -r1.42 files.c --- files.c 5 Aug 2015 07:10:03 - 1.42 +++ files.c 7 Aug 2023 21:53:45 - @@ -199,13 +199,14 @@ seq(FILE *fp, u_char **line) /* Long line - double size of buffer */ /* XXX: Check here for stupidly long lines */ buf_size *= 2; + ptrdiff_t offset = pos - buf; new_buf = realloc(buf, buf_size); if (!new_buf) err(2, "realloc of linebuf to %zu bytes failed", buf_size); - + end = new_buf + buf_size; - pos = new_buf + (pos - buf); + pos = new_buf + offset; buf = new_buf; } }
Re: CVS commit: src/sys/arch/sun2/dev
On 2023/08/04 20:18, Rin Okuyama wrote: Module Name:src Committed By: rin Date: Fri Aug 4 11:18:18 UTC 2023 Modified Files: src/sys/arch/sun2/dev: sc_mbmem.c Log Message: sun2/sc(4): Fix panic due to wrong ENOMEM for DMA buffer Use kmem_zalloc(9) for sc->sc_dma_handles[] to zero-initialize flags. XXX Will soon apply to sun3. Correction: sun3 is not affected. "sc(4) at vme(4)" has been fixed.
re: CVS commit: src/share/mk
> > Ah, I only thought about "obvious" impl. Thank you for kind
> > explanation! I will revert them for now.
>
> We should fix those cases that gcc12 found.
i was able to fix some of them with some code to apply offsets
generated before realloc() or free(), but not in all case.
a couple of them were pretty difficult because the alloc and
the re-align were not close together.
i was going to post about this soon, this is about 10% of the
remaining files i have in my tree... i actually don't think
the ${CC_..} change was worth reverting, as long as it was
using the "-Wno-error=..." method -- ie, the warnings are
still displayed, but not as errors (vs, the "-Wno-..." method
which elides the warning entirely.)
ie, i agree we should fix all these, i'm just not against
using the error disable as a temporary measure.
thanks.
.mrg.
Re: CVS commit: src/sys/arch
On Thu, Aug 03, 2023 at 08:16:31AM +, matthew green wrote: > Module Name: src > Committed By: mrg > Date: Thu Aug 3 08:16:31 UTC 2023 > > Modified Files: > src/sys/arch/evbarm/gumstix: gumstix_machdep.c > src/sys/arch/evbarm/ixm1200: ixm1200_machdep.c > src/sys/arch/hpcarm/hpcarm: pxa2x0_hpc_machdep.c sa11x0_hpc_machdep.c > src/sys/arch/hppa/stand: Makefile.buildboot > src/sys/arch/m68k/m68k: regdump.c > src/sys/arch/macppc/macppc: cpu.c > > Log Message: > ignore "-Warray-bounds" for various low level platform code that knows > how something is setup but technically is undefined behaviour. the > most common here is "extern int end;" and then using offsets of "" > that are outside the bounds of this 4-byte integer. > > these uses are almost certainly all OK in reality. Are you sure that GCC's optimizer is not going to break any of those assumptions? We had to go through quite some trouble in crtbegin.c for similar patterns? Joerg
Re: CVS commit: src/share/mk
On Thu, Aug 03, 2023 at 23:30:31 +0900, Rin Okuyama wrote: > On 2023/08/03 23:23, Valery Ushakov wrote: > > On Thu, Aug 03, 2023 at 13:33:27 +, Rin Okuyama wrote: > > > > > -Wuse-after-free for GCC 12 is premature. It fires on a common idiom: > > > > > > newbuf = realloc(buf, size); > > > p = newbuf + (p - buf); > > > > > > Let shut this up for GCC 12 (with hoping it gets improved for 13!). > > > > C99 says > > > > J.2 Undefined behavior > > > > [#1] The behavior is undefined in the following > > circumstances: > > [...] > > -- The value of a pointer to an object whose lifetime has > > ended is used (6.2.4). > > > > > > Yes, for the "obvious" implementation of pointers as addresses the > > above idiom happens to work, but it doesn't make that idiom any less > > UB. > > Ah, I only thought about "obvious" impl. Thank you for kind > explanation! I will revert them for now. We should fix those cases that gcc12 found. While it may seem like a stretch of imagination (e.g. compiling C to JVM or something like that, where the pointer is actually a nontrivial object), "fat" function pointers on itanium were a mundane thing and caused their fair share of problems for code that naively assumed trivial "address-only" pointers. I would imagine UB sanitizers will trip up on that idiom too... Thanks! -uwe
Re: CVS commit: src/share/mk
On 2023/08/03 23:23, Valery Ushakov wrote: On Thu, Aug 03, 2023 at 13:33:27 +, Rin Okuyama wrote: -Wuse-after-free for GCC 12 is premature. It fires on a common idiom: newbuf = realloc(buf, size); p = newbuf + (p - buf); Let shut this up for GCC 12 (with hoping it gets improved for 13!). C99 says J.2 Undefined behavior [#1] The behavior is undefined in the following circumstances: [...] -- The value of a pointer to an object whose lifetime has ended is used (6.2.4). Yes, for the "obvious" implementation of pointers as addresses the above idiom happens to work, but it doesn't make that idiom any less UB. Ah, I only thought about "obvious" impl. Thank you for kind explanation! I will revert them for now. Thanks, rin
Re: CVS commit: src/share/mk
On Thu, Aug 03, 2023 at 13:33:27 +, Rin Okuyama wrote: > -Wuse-after-free for GCC 12 is premature. It fires on a common idiom: > > newbuf = realloc(buf, size); > p = newbuf + (p - buf); > > Let shut this up for GCC 12 (with hoping it gets improved for 13!). C99 says J.2 Undefined behavior [#1] The behavior is undefined in the following circumstances: [...] -- The value of a pointer to an object whose lifetime has ended is used (6.2.4). Yes, for the "obvious" implementation of pointers as addresses the above idiom happens to work, but it doesn't make that idiom any less UB. -uwe
Re: CVS commit: src
Let's not make a mountain out of a molehill. Thanks Ryo for looking into it and the changes are good. In fact I had forgotten to commit the: M dist/libsanitizer/sanitizer_common/sanitizer_platform_limits_netbsd.h And the others worked for me because of the compat name stuff that I added as Matt requested. I will apply the rest of Ryo's patch and hopefully we can put it behind us. Best, christos > On Aug 2, 2023, at 2:38 AM, Ryo ONODERA wrote: > > Hi, > > I have the same build failure. > I think that there are some typos and inconsistencies. > > With the attached patch, I can finish build.sh build successfully. > > Thank you. > > On Wed, Aug 2, 2023 at 2:32 AM Christos Zoulas wrote: >> >> I am looking into it. With new code that restores the old names it should >> all work, unless I have typo that I have not found yet. >> >> christos >> >> > > > -- > Ryo ONODERA // r...@tetera.org > PGP fingerprint = 82A2 DC91 76E0 A10A 8ABB FD1B F404 27FA C7D1 15F3 > signature.asc Description: Message signed with OpenPGP
Re: CVS commit: src
Hi, I have no strong opinion about this change. I just want to unbreak the build to test the latest epoll(2) related side effects for pkgsrc packages. Thank you. On Wed, Aug 2, 2023 at 4:20 PM Taylor R Campbell wrote: > > Let's please just revert all these ioctl renames now, and go back to > the drawing board, rather than continue to dig ourselves into a deeper > hole with unnecessary incremental cosmetic changes that are breaking > all the builds. > > I generally agree that calling something plain `old' is bad and the > proliferation of them into sanitizers is also bad, but flailing around > with broken builds and without a clear plan for how this should fit > together and be maintained is much worse. -- Ryo ONODERA // r...@tetera.org PGP fingerprint = 82A2 DC91 76E0 A10A 8ABB FD1B F404 27FA C7D1 15F3
Re: CVS commit: src
Let's please just revert all these ioctl renames now, and go back to the drawing board, rather than continue to dig ourselves into a deeper hole with unnecessary incremental cosmetic changes that are breaking all the builds. I generally agree that calling something plain `old' is bad and the proliferation of them into sanitizers is also bad, but flailing around with broken builds and without a clear plan for how this should fit together and be maintained is much worse.
re: CVS commit: src
Ryo ONODERA writes: > I have the same build failure. > I think that there are some typos and inconsistencies. > > With the attached patch, I can finish build.sh build successfully. this looks like what i expected. the before / after showed fewer of the new names. please commit. we'll need similar fix for new gcc, but it's isn't building the sanitizer libraries yet. .mrg.
Re: CVS commit: src
Hi, I have the same build failure. I think that there are some typos and inconsistencies. With the attached patch, I can finish build.sh build successfully. Thank you. On Wed, Aug 2, 2023 at 2:32 AM Christos Zoulas wrote: > > I am looking into it. With new code that restores the old names it should all > work, unless I have typo that I have not found yet. > > christos > > -- Ryo ONODERA // r...@tetera.org PGP fingerprint = 82A2 DC91 76E0 A10A 8ABB FD1B F404 27FA C7D1 15F3 libsanitizer.diff Description: Binary data
Re: CVS commit: src
Bumped. christos > On Aug 1, 2023, at 2:17 AM, matthew green wrote: > > "Christos Zoulas" writes: >> Module Name: src >> Committed By:christos >> Date:Mon Jul 31 17:38:28 UTC 2023 >> >> Modified Files: >> src/distrib/sets/lists/comp: mi >> src/include: wchar.h >> src/lib/libc/string: Makefile.inc wmemchr.3 >> Added Files: >> src/lib/libc/string: wmempcpy.c >> >> Log Message: >> new gdb needs wmempcpy, give it to it. > > libc bump missing with this change. > > > .mrg. signature.asc Description: Message signed with OpenPGP
