Re: lint warning about extern (was: Re: CVS commit: src)
On Tue, Mar 28, 2023 at 22:13:25 +0200, Roland Illig wrote: > Am 28.03.2023 um 19:52 schrieb Valery Ushakov: > > On Tue, Mar 28, 2023 at 14:44:36 +, Roland Illig wrote: > > > > > lint: warn about extern declarations outside headers > > > > vs. > > > > > https://mail-index.netbsd.org/tech-userlevel/2023/03/15/msg013727.html > > > > which is nested extern declaration inside a function body. > > > > I.e. the two lines of this log message contradict each other. So > > which one of those does this change actually address? > > Oops, I implemented an entirely unrelated warning in message 351, which > is about a function or object declaration that is outside a header and > thus may lead to inconsistent definitions when it is declared > differently in several translation units. This message helps finding > cases where identifiers are needlessly exported instead of being > declared 'static'. > > I now added another message 352 that covers the case from the > discussion, which is about declarations inside function bodies with > storage class 'extern'. > > Sorry for the confusion. Thanks for the clarification and a quick follow up! -uwe
Re: CVS commit: src
On Tue, Mar 28, 2023 at 14:44:36 +, Roland Illig wrote: > lint: warn about extern declarations outside headers vs. > https://mail-index.netbsd.org/tech-userlevel/2023/03/15/msg013727.html which is nested extern declaration inside a function body. I.e. the two lines of this log message contradict each other. So which one of those does this change actually address? -uwe
Re: CVS commit: src/sys/arch/sparc64/sparc64
On Mon, Feb 20, 2023 at 15:41:04 +0100, Martin Husemann wrote: > On Mon, Feb 20, 2023 at 05:15:33PM +0300, Valery Ushakov wrote: > > them up, b/c you cannot amend that comment. To add to the fun, I > > think releng scripts just clone the commit message on pull ups, so > > that comment gets splattered all over the target branches too. > > Yes - I try to manually remove them (which is easy if they are at the > end of the last commit log in the batch) durint the pullup. Data point: $ hg log -b netbsd-9 | grep 'XXX.*pullup.*9' | wc -l 74 -uwe
Re: CVS commit: src/sys/arch/sparc64/sparc64
On Mon, Feb 20, 2023 at 22:35:40 +0700, Robert Elz wrote: > Date:Mon, 20 Feb 2023 16:47:01 +0300 > From: Valery Ushakov > Message-ID: > > | I wonder if we should stop abusing commit messages as pull-up > | reminders. These XXX will not convey any useful information a few > | months down the line... > > I think they're useful (if only I remembered to add them all the > times I should) - it allows someone looking at the commit log to > easily determine that the change is also intended for another branch. > Then one can check and see if it happened or not, and if not, send > a reminder if it is important/needed. > > If the pullup annotation is missing, I tend to assume that the change > is not intended to be pulled up - either it isn't applicable, or is > something new that isn't appropriate for older releases. > > That's why I will sometimes even include pullup annotations for ancient > versions of NetBSD, even though I know they will never happen (never get > submitted, much less acted upon) - just as an indication that the problem > being fixed exists from long ago. In that case they have no business being "XXX" :) -uwe
Re: CVS commit: src/sys/arch/sparc64/sparc64
On Mon, Feb 20, 2023 at 13:57:32 +, Taylor R Campbell wrote: > > > XXX pullup-8 > > > XXX pullup-9 > > > XXX pullup-10 > > > > I wonder if we should stop abusing commit messages as pull-up > > reminders. These XXX will not convey any useful information a few > > months down the line... > > Happy to try a better mechanism if you have suggestions, but this is > the best one I've found so far! If I don't mark commits this way, I'm > almost guaranteed not to pull them up. I'm not sure any exists even with modern VCS. May be something like a branch that is not closed until it is merged (pulled-up) to all the intended destination (but then the very notion of the closed branch requires something more modern than CVS). The problem with the XXX in the commit message is that you don't easily know if you _did_ pull them up, b/c you cannot amend that comment. To add to the fun, I think releng scripts just clone the commit message on pull ups, so that comment gets splattered all over the target branches too. -uwe
Re: CVS commit: src/sys/arch/sparc64/sparc64
On Mon, Feb 20, 2023 at 13:30:23 +, Taylor R Campbell wrote: > Module Name: src > Committed By: riastradh > Date: Mon Feb 20 13:30:23 UTC 2023 > > Modified Files: > src/sys/arch/sparc64/sparc64: lock_stubs.s > > Log Message: > sparc64: Add missing LoadStore ordering for mutex_enter stub. > > XXX pullup-8 > XXX pullup-9 > XXX pullup-10 I wonder if we should stop abusing commit messages as pull-up reminders. These XXX will not convey any useful information a few months down the line... -uwe
Re: .Bl -width (Was: CVS commit: src/sbin/shutdown)
On Wed, Feb 15, 2023 at 13:40:42 +0300, Valery Ushakov wrote: > On Wed, Feb 15, 2023 at 01:55:17 +, Jan Schaumann wrote: > > > Module Name:src > > Committed By: jschauma > > Date: Wed Feb 15 01:55:17 UTC 2023 > > > > Modified Files: > > src/sbin/shutdown: shutdown.8 > > > > Log Message: > > adjust width of flag arg to align more nicely PS: And the date reflects the semantic contents of the man page, not the formatting tweaks or trivial typo fixes, so the date bump was unnecessary. -uwe
Re: CVS commit: src/share/man/man4
On Wed, Feb 08, 2023 at 12:09:54 -0500, David H. Gutteridge wrote: > > In postscript output Pq has different spacing than literal () (in > > other entries around it). > > I wondered if there could be a difference. Which format do you prefer? > (My inclination would just be to use literal parentheses, being less > effort.) The correct markup is to use .Vt for the type and I've grown to like Pq to give the text inside the parens a bit of breathing space, so I'd say Pq Vt u_int But don't waste your time on this. I'll try to give this man page a cleanup this weekend when I have more time. -uwe
Re: CVS commit: src/share/man/man4
On Tue, Feb 07, 2023 at 01:17:41 +, David H. Gutteridge wrote: > Module Name: src > Committed By: gutteridge > Date: Tue Feb 7 01:17:41 UTC 2023 > > Modified Files: > src/share/man/man4: bpf.4 > > Log Message: > bpf.4: fix a garbled item heading > > Make the BIOCSDIRECTION & BIOCGDIRECTION entry like those around it. In postscript output Pq has different spacing than literal () (in other entries around it). -uwe
Re: CVS commit: src/sys/dev/wscons
On Wed, Jan 18, 2023 at 12:02:17 -0500, Christos Zoulas wrote: > Module Name: src > Committed By: christos > Date: Wed Jan 18 17:02:17 UTC 2023 > > Modified Files: > src/sys/dev/wscons: wsemul_vt100_subr.c > > Log Message: > Add rin, indn, vpa, hpa, and cbt terminfo capabilities (Crystal Kolipe) They probably need to be added to the terminfo description too. -uwe
Re: CVS commit: xsrc/external/mit/xorg-cf-files/dist
On Tue, Jan 17, 2023 at 05:37:05 +, matthew green wrote: > Module Name: xsrc > Committed By: mrg > Date: Tue Jan 17 05:37:05 UTC 2023 > > Modified Files: > xsrc/external/mit/xorg-cf-files/dist: Imake.tmpl > > Log Message: > pull over fix from pkgsrc xorg-cf-files and avoid ar's "l" flag. That doesn't seem to handle all the cases where "l" flag is used (ArAddCmd, ArExtCmd). Might be easier to either define HasLargeTmp which seems to be *the* knob to control "l" flag to ar to begin with, or to factor out the test for "supports l flag" so that the unwieldy SystemV4 || LinuxBinUtilsMajorVersion || defined(NetBSDArchitecture) is not repeated multiple times. -uwe
Re: CVS commit: src
On Sun, Aug 28, 2022 at 10:48:17 +, Harold Gutch wrote: > Change back various occurrences of \*[Le], \*[Ge] (less/greater equal) > and \*(ua (upwards arrow) to literal "<=", ">=" and "^" whenever > appropriate (e.g., in code examples). Thanks. Using your commit as a pretext (and in no way intended as censure) ... I wonder if we should switch to consistently use \(ha (a groff extension also understood by heirloom) for ascii circumflex. Troff turns plain ^ into circumflex accent which is visually very small and you cannot find it, obviously, when you search pdf output for "^". Check out e.g. groff -Tps -mandoc csh.1 > csh.ps && ps2pdf csh.ps csh.pdf (that postscript output has a lot of other problems, b/c examples are not in monospaced font, etc, which makes the ^ size problem even worse visually). -uwe
Re: CVS commit: src
On Sat, Aug 27, 2022 at 21:53:39 +, David A. Holland wrote: > Log Message: > Attach tradcpp to the build. Thanks! -uwe
Re: CVS commit: src/tests/lib/libc/sys
On Mon, Aug 01, 2022 at 15:48:40 +, Robert Elz wrote: > Module Name: src > Committed By: kre > Date: Mon Aug 1 15:48:40 UTC 2022 > > Modified Files: > src/tests/lib/libc/sys: Makefile > > Log Message: > Provide _GNU_SOURCE for t_clone now that is required to make clone() > visible. The test uses __clone(), not clone() and a followup fix made __clone() visible under plain _NETBSD_SOURCE again - which is the right thing, IMO. So this change is not necessary (the test only uses __clone()). -uwe
Re: CVS commit: xsrc/external/mit/xorg-server/dist/hw/xfree86/drivers/modesetting
On Fri, Jul 15, 2022 at 05:08:56 +, matthew green wrote: > Modified Files: > xsrc/external/mit/xorg-server/dist/hw/xfree86/drivers/modesetting: > drmmode_display.c > > Log Message: > use "1ULL << 32" for a 64 bit member, not 1UL. We should be able to use UINT64_C(1) here, or? -uwe
Re: CVS commit: src/external/historical/nawk/bin
On Wed, Jul 06, 2022 at 20:59:43 -, Christos Zoulas wrote: > >> Thanks uwe! Are you planning to upstream all these changes? > > > >This is our mdocified bin/awk.1, upstream is dist/awk.1 that is > >-man-old, so I'm not sure how to proceed. Also most of the changes I > >made are strictly mdoc related. POSIX has quite a bit more elaborate > >awk man page that spells out many details, importing some more of that > >might be good but it's somewhat heavy, prescriptive prose and requires > >some editing for better readibility, IMHO - not something I feel > >qualified to do. > > Ok thanks. I will check with upstream on replacing theirs with ours. Not without some editorial efforts. OpenBSD has their own additions to the man page, e.g. documenting that literal space as FS value (the default) is magic, etc. It would be nice to collate those. I don't have a fresh FreBSD tree handy to check their version. -uwe
Re: CVS commit: src/external/historical/nawk/bin
On Tue, Jul 05, 2022 at 20:20:09 -, Christos Zoulas wrote: > In article <20220705184003.aa082f...@cvs.netbsd.org>, > Valeriy E. Ushakov wrote: > >-=-=-=-=-=- > > > >Module Name: src > >Committed By:uwe > >Date:Tue Jul 5 18:40:03 UTC 2022 > > > >Modified Files: > > src/external/historical/nawk/bin: awk.1 > > > >Log Message: > >awk(1): consistent Capitalization. > > Thanks uwe! Are you planning to upstream all these changes? This is our mdocified bin/awk.1, upstream is dist/awk.1 that is -man-old, so I'm not sure how to proceed. Also most of the changes I made are strictly mdoc related. POSIX has quite a bit more elaborate awk man page that spells out many details, importing some more of that might be good but it's somewhat heavy, prescriptive prose and requires some editing for better readibility, IMHO - not something I feel qualified to do. PS: $ nroff -C -man-old man/1/charon Hashtag TrueStory, hashtag NewOptimism -uwe
Re: CVS commit: src/external/historical/nawk/bin
On Mon, Jul 04, 2022 at 20:06:45 +0200, Roland Illig wrote:
> Am 04.07.2022 um 13:14 schrieb Valery Ushakov:
> > On Mon, Jul 04, 2022 at 08:04:57 +0200, Roland Illig wrote:
> >
> > > 04.07.2022 01:54:16 Valery Ushakov :
> > > > On Mon, Jul 04, 2022 at 00:07:23 +0200, Roland Illig wrote:
> > > >
> > > > > Am 03.07.2022 um 21:55 schrieb Valery Ushakov:
> > > > > > On Sun, Jul 03, 2022 at 10:56:22 +, Roland Illig wrote:
> > > > > >
> > > > > > > Module Name: src
> > > > > > > Committed By: rillig
> > > > > > > Date: Sun Jul 3 10:56:22 UTC 2022
> > > > > > >
> > > > > > > Modified Files:
> > > > > > > src/external/historical/nawk/bin: awk.1
> > > > > > >
> > > > > > > Log Message:
> > > > > > > awk.1: remove trailing space in output of 'echo' example program
> > > > > >
> > > > > > This is cure worse than the desease. Please revert.
> > > > >
> > > > > Why is it worse?
> > > >
> > > > It's ugly
> > >
> > > Why is it ugly?
> > >
> > > > and complicated (for an example),
> > >
> > > Why is it complicated? It's still only 3 lines of code.
> > >
> > > > it obscures the point this example tries to make.
> > >
> > > What is this (single?) point this example tries to make? To me, it
> > > was how to write a BEGIN program that uses ARGV, and my rewritten
> > > code still illustrates this.
> >
> > You have turned a trivial for loop that requires no mental bandwidth
> > to skim over into a code review errand with complex case analysis and
> > ugly inverted 1 < ARGC to boot.
>
> I intentionally wrote '1 < ARGC' instead of 'ARGC > 1' to make the
> condition as close as possible to the condition in the 'for' loop. If
> it weren't for this symmetry, I would of course have written it in the
> subject-first manner.
[...]
> > but this case is not
> > about code doing something specified with aesthetic considerations
> > being secondary to its actually doing the job. Here taste is an
> > important factor b/c it's not code, but a man page - a literally work
> > really.
>
> That's exactly my point. The manual page is our reference
> documentation, and as such, it should demonstrate and teach best
> practices. Presenting a program that is _almost correct_ misses this
> point, and I don't want to see any code derived from an _almost correct_
> example. Mixing such code with other _almost correct_ example code will
> quickly lead to programs with bugs everywhere.
>
> An example for this would be the common usage of functions,
> which really many people get wrong, either by reading sloppily produced
> teaching material or incomplete documentation or by copying code
> snippets that seem to work.
So this needs the symmetry with the next line and the reader needs to
register and process that symmetry. Then the fun part comes, the
reader needs to match the two bodies (if and for) and figure out if
and how they differ and find that visually elusive whitespace. And
this has already snowballed into quite a code review errand.
ctype and the C integer promotion rules that it touches (often
inappropriately) go to some pretty dark corners of the C standard.
Proper examples of correct use are essential for that man page and
those examples are kinda the focal point of it.
Here the point of the example is a BEGIN-only program that doesn't
process any input. Any content of that BEGIN action is rather
unimportant, a kind of "lorem ipsum" filler almost. So turning it
into an exercise in correct re-implementation of echo(1) draws
attention to the wrong thing. Consider that extra space in the output
to be poetic license.
Posix has the echo example as:
BEGIN {
for (i = 1; i < ARGC; ++i)
printf("%s%s", ARGV[i], i==ARGC-1?"\n":" ")
}
which I would still consider ugly :). I would rather use instead:
BEGIN { for (i = 1; i < ARGC; ++i) $i = ARGV[i]; print }
that, as a bonus, demonstrates $var field references (that the current
man page doesn't mention at all) and the magic $0 "reassembly".
> > PS: BTW, you also eliminated the "exit" at the end of the BEGIN
> > action. This is not specified by POSIX and happens to work on all
> > major three - nawk, mawk, gawk, though only gawk seems to document
> > this and only in the info file, not its man page (though its man page
> > has
Re: CVS commit: src/external/historical/nawk/bin
On Sun, Jul 03, 2022 at 22:55:48 +0300, Valery Ushakov wrote: > On Sun, Jul 03, 2022 at 10:56:22 +, Roland Illig wrote: > > > Module Name:src > > Committed By: rillig > > Date: Sun Jul 3 10:56:22 UTC 2022 > > > > Modified Files: > > src/external/historical/nawk/bin: awk.1 > > > > Log Message: > > awk.1: remove trailing space in output of 'echo' example program > > This is cure worse than the desease. Please revert. PS: BTW, you also eliminated the "exit" at the end of the BEGIN action. This is not specified by POSIX and happens to work on all major three - nawk, mawk, gawk, though only gawk seems to document this and only in the info file, not its man page (though its man page has exampes that rely on this behaviour). E.g. solaris /usr/bin/awk will still wait for input to consume and discard it. -uwe
Re: CVS commit: src/external/historical/nawk/bin
On Mon, Jul 04, 2022 at 08:04:57 +0200, Roland Illig wrote: > 04.07.2022 01:54:16 Valery Ushakov : > > On Mon, Jul 04, 2022 at 00:07:23 +0200, Roland Illig wrote: > > > >> Am 03.07.2022 um 21:55 schrieb Valery Ushakov: > >>> On Sun, Jul 03, 2022 at 10:56:22 +, Roland Illig wrote: > >>> > >>>> Module Name: src > >>>> Committed By: rillig > >>>> Date: Sun Jul 3 10:56:22 UTC 2022 > >>>> > >>>> Modified Files: > >>>> src/external/historical/nawk/bin: awk.1 > >>>> > >>>> Log Message: > >>>> awk.1: remove trailing space in output of 'echo' example program > >>> > >>> This is cure worse than the desease. Please revert. > >> > >> Why is it worse? > > > > It's ugly > > Why is it ugly? > > > and complicated (for an example), > > Why is it complicated? It's still only 3 lines of code. > > > it obscures the point this example tries to make. > > What is this (single?) point this example tries to make? To me, it > was how to write a BEGIN program that uses ARGV, and my rewritten > code still illustrates this. You have turned a trivial for loop that requires no mental bandwidth to skim over into a code review errand with complex case analysis and ugly inverted 1 < ARGC to boot. I realize this is de gustibus and if you don't see this new code that way I probably cannot make you see it that way, but this case is not about code doing something specified with aesthetic considerations being secondary to its actually doing the job. Here taste is an important factor b/c it's not code, but a man page - a literally work really. > > The point is not to write a perfect echo(1) clone in awk. > > If that had been my goal, I would have implemented the -n option as > well, at which point the code would have grown large enough to be > extracted into a separate file. I didn't do that though. Well, what *was* your goal? To be "technically correct, the best kind of correct"? I'm pretty sure that whoever wrote that example did know how to write a "proper" echo in awk, but chose not to deliberately. -uwe
Re: CVS commit: src/external/historical/nawk/bin
On Mon, Jul 04, 2022 at 00:07:23 +0200, Roland Illig wrote: > Am 03.07.2022 um 21:55 schrieb Valery Ushakov: > > On Sun, Jul 03, 2022 at 10:56:22 +, Roland Illig wrote: > > > > > Module Name: src > > > Committed By: rillig > > > Date: Sun Jul 3 10:56:22 UTC 2022 > > > > > > Modified Files: > > > src/external/historical/nawk/bin: awk.1 > > > > > > Log Message: > > > awk.1: remove trailing space in output of 'echo' example program > > > > This is cure worse than the desease. Please revert. > > Why is it worse? It's ugly, and complicated (for an example), and it obscures the point this example tries to make. The point is not to write a perfect echo(1) clone in awk. -uwe
Re: CVS commit: src/external/historical/nawk/bin
On Sun, Jul 03, 2022 at 10:56:22 +, Roland Illig wrote: > Module Name: src > Committed By: rillig > Date: Sun Jul 3 10:56:22 UTC 2022 > > Modified Files: > src/external/historical/nawk/bin: awk.1 > > Log Message: > awk.1: remove trailing space in output of 'echo' example program This is cure worse than the desease. Please revert. -uwe
Re: CVS commit: src/usr.bin/man
On Sat, Jun 18, 2022 at 02:19:07 +, David H. Gutteridge wrote: > Module Name: src > Committed By: gutteridge > Date: Sat Jun 18 02:19:07 UTC 2022 > > Modified Files: > src/usr.bin/man: man.conf.5 > > Log Message: > man.conf.5: add details about the machine line and search order > > Also, tweak some grammar, style, and markup while here. > > > To generate a diff of this commit: > cvs rdiff -u -r1.27 -r1.28 src/usr.bin/man/man.conf.5 Please, could you rephrase the bit about the _ keyword. It is not quite clear on the first reading which is the keyword and which is the value (epecially in text output where formatting (wrong anyway :) doesn't provide a hint). Using "value" to talk about the keyword doesn't help too. Also, "commonly" in the added phrase is misleading I'd say. Commonly implies "not always", which is false - we suppply x86 as an extra subdir in the default man.conf. I know that passage was not very clear to begin with, but you touch it you buy it :). Not being a native speaker I don't dare touching it, I'm afraid to make more mess. TIA! -uwe
Re: CVS commit: xsrc/external/mit
On Fri, May 27, 2022 at 15:22:45 +, nia wrote: > On Fri, May 27, 2022 at 06:10:22PM +0300, Valery Ushakov wrote: > > On Fri, May 27, 2022 at 14:23:23 +, Nia Alarie wrote: > > > > > Module Name: xsrc > > > Committed By: nia > > > Date: Fri May 27 14:23:23 UTC 2022 > > > > > > Modified Files: > > > xsrc/external/mit/xdm/dist/config: Xsession.in > > > xsrc/external/mit/xinit/dist: xinitrc.cpp > > > > > > Log Message: > > > In xterm, use TrueType fonts rather than Spleen at higher DPIs. > > > > >From a quick look - doesn't that also need renderFont set to true? > > > > -uwe > > "If the faceName resource is set, then start by using > the TrueType font rather than the bitmap font." > > Should be fine. Ah, that seems to have been changed in the version we have in -current. The older version said something different there. Sorry for the noise. -uwe
Re: CVS commit: xsrc/external/mit
On Fri, May 27, 2022 at 14:23:23 +, Nia Alarie wrote: > Module Name: xsrc > Committed By: nia > Date: Fri May 27 14:23:23 UTC 2022 > > Modified Files: > xsrc/external/mit/xdm/dist/config: Xsession.in > xsrc/external/mit/xinit/dist: xinitrc.cpp > > Log Message: > In xterm, use TrueType fonts rather than Spleen at higher DPIs. >From a quick look - doesn't that also need renderFont set to true? -uwe
Re: CVS commit: src
On Sat, May 14, 2022 at 14:02:08 +, Valeriy E. Ushakov wrote: > Module Name: src > Committed By: uwe > Date: Sat May 14 14:02:08 UTC 2022 > > Modified Files: > src/distrib/sets/lists/base: mi > src/doc: 3RDPARTY > src/share/wscons/fonts: Makefile > Added Files: > src/share/wscons/fonts: ter-i16b.wsf.uue ter-i16n.wsf.uue > ter-i16v.wsf.uue ter-i20n.wsf.uue ter-i24n.wsf.uue ter-i32n.wsf.uue > > Log Message: > Terminus Font: Import IBM-encoded versions from 4.49.1 > > Terminus Font is a clean, fixed width bitmap font, designed for long > (8 and more hours per day) work with computers. > > The font has a very good script coverage and subsets with other > encodings can be imported in the future. > > This commit imports IBM437 encoded (WSDISPLAY_FONTENC_ISO) subsets > converted to WSF format. wsfont metadata are stored in the files, so > you can load them without any additional arguments to wsfontload(8). *sigh* The log should say "WSDISPLAY_FONTENC_IBM", sorry. -uwe
Re: CVS commit: xsrc/external/mit
On Mon, May 09, 2022 at 07:00:15 +, Nia Alarie wrote: > Module Name: xsrc > Committed By: nia > Date: Mon May 9 07:00:15 UTC 2022 > > Modified Files: > xsrc/external/mit/xdm/dist/config: Xsession.in > xsrc/external/mit/xinit/dist: xinitrc.cpp > > Log Message: > Default X session: In xterm and xman at lower DPIs, use misc-fixed > instead of spleen for greater readability. Reasoning: misc-fixed > has native bold variants. You can avoid code duplication and multiple invocations of xrdb by putting the default resources into a file that both xinitrc and Xsession can load and using the fact that xrdb uses CPP on its input. E.g. $ xrdb -n -DFontSize=22 #if FontSize + 0 > 18 # define FONT Large #else # define FONT Small #endif *font: FONT gets you *font: Large -uwe
Re: CVS commit: src/sys/arch
On Sat, May 07, 2022 at 04:12:55 +, Rin Okuyama wrote: > Module Name: src > Committed By: rin > Date: Sat May 7 04:12:55 UTC 2022 > > Modified Files: > src/sys/arch/evbppc/dht: locore.S > src/sys/arch/powerpc/include: spr.h > > Log Message: > Instead of hard-coding SPR# for CCR0, define SPR_CCR0 in > and use it. > > Idea from uwe@, thanks! > (and sorry for delayed response!) Note that we already have it defined (along with bunch of others) in sys/arch/powerpc/include/ibm4xx/spr.h:74 -uwe
Re: CVS commit: src/lib/libcurses
On Sun, Jan 16, 2022 at 10:30:45 +, Roland Illig wrote: > Modified Files: > src/lib/libcurses: addbytes.c ins_wstr.c > > Log Message: > libcurses: remove unreachable statements That summary sounds kinda misleading to me. It's technically true - the commit removes unreachable *empty* statements - accidentally doubled semicolons in return foo;; and the like - but it makes it sound much more ominous by failing to mention that fact. I would have skipped a change summarized, e.g., as "g/c duplicate semicolons" or something like that that actually captured the essense of the change correctly. I felt compelled to go and take a look at something described as "remove unreachable statements". Please, can you be more careful in wording those? -uwe
Re: CVS commit: src/lib/libcurses
On Thu, Jan 06, 2022 at 06:18:13 +, Brett Lymn wrote:
> Module Name: src
> Committed By: blymn
> Date: Thu Jan 6 06:18:13 UTC 2022
>
> Modified Files:
> src/lib/libcurses: slk.c
>
> Log Message:
> Properly size and array to hold the larget return from wctomb.
I'm not sure this is correct. You are introducing a variable length
array here.
POSIX says that
The header shall define the following macro which shall
expand to a positive integer expression with type size_t:
{MB_CUR_MAX}
Maximum number of bytes in a character specified by the current
locale (category LC_CTYPE).
the important exegetical fact about this passage is that it says
"integer expression", not "integer constant expression".
MB_LEN_MAX is maximum MB_CUR_MAX and "shall be suitable for use in #if
preprocessing directives".
I think this should be reverted.
-uwe
Re: CVS commit: xsrc/external/mit/xinit/dist
On Wed, Dec 29, 2021 at 07:42:42 +0300, Valery Ushakov wrote:
> On Wed, Dec 29, 2021 at 15:09:08 +1100, matthew green wrote:
>
> > "Nia Alarie" writes:
> > > Module Name: xsrc
> > > Committed By: nia
> > > Date: Tue Dec 28 11:48:52 UTC 2021
> > >
> > > Modified Files:
> > > xsrc/external/mit/xinit/dist: xinitrc.cpp
> > >
> > > Log Message:
> > > COLOR is not a C preprocessor macro :|
> >
> > this used to work a long long time ago. back when
> > sunos4 was not entirely obsolete :-)
>
> I actually still have #ifdef COLOR in my resources carried around from
> that time, but the it's been quite a long ago that I used an actual
> b/w (1bpp) display where this actually mattered. Any VAXstation users
> around? Also, why not arrange for COLOR to be defined?
Uhm, actually xrdb *does* define COLOR but it defines it without value
switch (visual->class) {
case StaticColor:
case PseudoColor:
case TrueColor:
case DirectColor:
AddSimpleDef(defs, "COLOR");
break;
}
so the right fix would be to just fix the #if in xinitrc to #ifdef
-uwe
Re: CVS commit: xsrc/external/mit/xinit/dist
On Wed, Dec 29, 2021 at 15:09:08 +1100, matthew green wrote: > "Nia Alarie" writes: > > Module Name:xsrc > > Committed By: nia > > Date: Tue Dec 28 11:48:52 UTC 2021 > > > > Modified Files: > > xsrc/external/mit/xinit/dist: xinitrc.cpp > > > > Log Message: > > COLOR is not a C preprocessor macro :| > > this used to work a long long time ago. back when > sunos4 was not entirely obsolete :-) I actually still have #ifdef COLOR in my resources carried around from that time, but the it's been quite a long ago that I used an actual b/w (1bpp) display where this actually mattered. Any VAXstation users around? Also, why not arrange for COLOR to be defined? -uwe
Re: CVS commit: src/usr.bin/m4/PSD.doc
On Tue, Dec 07, 2021 at 21:37:35 +0100, Roland Illig wrote: > Am 07.12.2021 um 20:59 schrieb Valery Ushakov: > > > If the point you wanted to make with this inarticulate statement was > > that we don't have m4.ms imported, then the proper thing to do is to > > import it > > Is there a way I could have known this? The manual had been absent > since 1993, therefore I assumed that after 28 years nobody would be > interested in adding it. That seemed to be the wrong conclusion. This was discussed on tech-doc briefly not *that* long ago (which is why I had this in my tree uncommitted). Obviosly no-one can be expected to follow all the mailing lists closely, but asking on tech-doc first might have been a good idea. -uwe
Re: CVS commit: src/usr.bin/m4/PSD.doc
On Tue, Dec 07, 2021 at 19:28:19 +, Roland Illig wrote: > Removed Files: > src/usr.bin/m4/PSD.doc: Makefile > > Log Message: > m4: remove PSD.doc > > make: don't know how to make m4.ms. Stop That was uncalled for... The cited reason is bogus b/c we don't descend into this directory, so the fact that make fails there is not a problem. If the point you wanted to make with this inarticulate statement was that we don't have m4.ms imported, then the proper thing to do is to import it - which I was going to do after the last time this was brought up (I do have it in my tree), but apparently never got around to. -uwe
Re: CVS commit: src/bin/echo
On Thu, Nov 11, 2021 at 05:03:41 +0700, Robert Elz wrote:
> Date:Wed, 10 Nov 2021 22:17:05 +0300
> From: Valery Ushakov
> Message-ID:
>
> | > in the sense that simply falling out of main() is exit(0)?
> |
> | Surprisingly - yes.
>
> That's appalling, but perhaps not surprising.
>
> It breaks code which believed what was promised, and did return n
> (n != 0) instead of exit(n).
The main can still return an int explicitly.
If the return type of the main function is a type compatible with
int, a return from the initial call to the main function is
equivalent to calling the exit function with the value returned by
the main function as its argument;
It's specifically the sloppy
int main() {}
without explicit return that gets a dispensation.
-uwe
Re: CVS commit: src/bin/echo
On Wed, Nov 10, 2021 at 17:35:45 +, Robert Elz wrote: > And second, does C99 really promise: > Remove unnecessary call to exit(0); returning from main is equivalent > since C99. > in the sense that simply falling out of main() is exit(0)? Surprisingly - yes. Derek M. Jones in his "The New C Standard. An Economic and Cultural Commentary." notes about the relevant passage in the standard text: 5.1.2.2.3 Program termination [...] reaching the } that terminates the main function returns a value of 0. Commentary The standard finally having to bow to sloppy existing practices. I'd say the previous change to take advantage of this was profoudly misguided. -uwe
Re: CVS commit: src/lib/libcurses
On Mon, Jul 26, 2021 at 21:04:37 +, nia wrote: > On Mon, Jul 26, 2021 at 09:01:51PM +, nia wrote: > > On Mon, Jul 26, 2021 at 11:55:24PM +0300, Valery Ushakov wrote: > > > On Mon, Jul 26, 2021 at 20:17:10 +, Nia Alarie wrote: > > > > > > > Module Name:src > > > > Committed By: nia > > > > Date: Mon Jul 26 20:17:10 UTC 2021 > > > > > > > > Modified Files: > > > > src/lib/libcurses: curses_standout.3 > > > > > > > > Log Message: > > > > The BUGS sections is incorrect again for "modern" terminals. > > > > > > > > For example, wscons and xterm both display standout differently to bold. > > > > > > Please, revert. The bugs section need to be changed to caveats (it's > > > not a bug, if it's by design) and fixed, not removed, as the sitution > > > it warns about is still very much pertinent. The fact that xterm > > > chose to use A_REVERSE video instead of A_BOLD is just an irrelevant > > > detail. > > > > > > -uwe > > > > Suggested wording? I was thinking: > > > > "Some modern terminals may display characters with the standout > > attribute set identically to those with the bold or reverse attribute > > set." > > ... A note that some legacy terminals may _only_ support standout > is probably pertinent too, given the discussion that spawned this. I'm not sure what is the discussion you are referring to, but it's not a matter of a simple rewording. "It's complicated", as it ties into a few other things, so ideally it needs a coordinated change to several man pages. Even the official "X/Open Curses, Issue 7" docs are not very good at explaining this and cross-referencing things properly, so you need to do a bit of legwork to find out. The two key points from the spec that the BUGS section tries to warn you about is (emphasis mine): A_STANDOUT - Best highlighting mode of the terminal The standend() and wstandend() functions turn off ALL attributes of the current or specified window. Roughly speaking "standout" is a kind of higher-level alais and it works differently than other attributes (standend). Mixing it with explicit use of other lower-level attributes is not a good idea. -uwe
Re: CVS commit: src/lib/libcurses
On Mon, Jul 26, 2021 at 20:17:10 +, Nia Alarie wrote: > Module Name: src > Committed By: nia > Date: Mon Jul 26 20:17:10 UTC 2021 > > Modified Files: > src/lib/libcurses: curses_standout.3 > > Log Message: > The BUGS sections is incorrect again for "modern" terminals. > > For example, wscons and xterm both display standout differently to bold. Please, revert. The bugs section need to be changed to caveats (it's not a bug, if it's by design) and fixed, not removed, as the sitution it warns about is still very much pertinent. The fact that xterm chose to use A_REVERSE video instead of A_BOLD is just an irrelevant detail. -uwe
Re: CVS commit: src/tests/lib/libcurses/director
On Tue, Apr 06, 2021 at 00:47:00 +, Roland Illig wrote: > The previous "table" was an insult to any reader. It was unsorted, > listed the functions shuffled, and was not even formatted consistently. That's pretty rude. Please, tone down your commit "messages". -uwe
Re: CVS commit: src/usr.bin/xlint
On Sat, Mar 27, 2021 at 01:44:07 +0100, Roland Illig wrote:
> On 27.03.2021 00:16, Valery Ushakov wrote:
> > On Sat, Mar 27, 2021 at 00:01:25 +0100, Roland Illig wrote:
> > > To me, writing 'sizeof expr' is clearer than 'sizeof(expr)' since
> > > 'sizeof' is not a function, same as with 'return'.
> > >
> > > Did I misinterpret the style guide in this regard?
> >
> > We do want it to look like a function call (and so the usual style
> > rules for the function call apply). Think of cases where it's used as
> > a subexpression, e.g.
> >
> > sizeof(expr) + 4
>
> The parentheses of a function call expression have highest precedence
> and bind to the left. The parentheses to the right of the sizeof
> operator do not bind at all though. Therefore I find it misleading to
> write them like in a function call.
>
> The following code demonstrates why I prefer to omit the parentheses
> around the argument to the 'sizeof' operator:
[...]
> /* misleading since it looks like a function call */
> printf("%zu\n", sizeof(argv)[3]);
[...]
> It was fun to find out that there actually _is_ a case where the
> different precedence matters. At first I had thought that the whole
> discussion would be academic anyway.
You cite examples worthy of IOCCC, but we already know C is
treacherous (things like relative precedence of bitwise and
comparison, etc). But most uses of sizeof are not like that.
Note also that the parentheses are required fort the sizeof(type-name)
variant. I.e. you cannot write sizeof int, only sizeof(int).
IOCCC cases aside, I still maintain that sizeof *p + 4 is strictly
less readable than sizeof(*p) + 4 and is nicely similar to
sizeof(uint32_t) + 4 etc
Derek Jones' "The New C Standard" (influential exegetical tour de
force) doesn't seem to explicitly recommend parentheses for the
unary-expr case but seems to consitently use it in its own text.
-uwe
Re: CVS commit: src/usr.bin/xlint
On Sat, Mar 27, 2021 at 00:01:25 +0100, Roland Illig wrote:
> > > Log Message:
> > > lint: in malloc calls, use 'sizeof *ptr' instead of 'sizeof(type)'
> >
> > style says "sizeof(" not "sizeof "
> >
> > * Casts and sizeof's are not followed by a space.
>
> There are several forms of writing sizeof:
>
> 1. sizeof expr
> 2. sizeof(expr)
> 3. sizeof (expr)
> 4. sizeof(type)
> 5. sizeof (type)
>
> I thought that the point of the rule you cited was to discourage forms 3
> and 5. Does the rule also discourage form 1, and if so, why?
>
> To me, writing 'sizeof expr' is clearer than 'sizeof(expr)' since
> 'sizeof' is not a function, same as with 'return'.
>
> Did I misinterpret the style guide in this regard?
We do want it to look like a function call (and so the usual style
rules for the function call apply). Think of cases where it's used as
a subexpression, e.g.
sizeof(expr) + 4
-uwe
Re: CVS commit: src/usr.bin/sed
On Thu, Mar 11, 2021 at 10:15:05 -0500, Christos Zoulas wrote: > Module Name: src > Committed By: christos > Date: Thu Mar 11 15:15:05 UTC 2021 > > Modified Files: > src/usr.bin/sed: main.c sed.1 > > Log Message: > Add -G to support GNU extensions This is inverse of m4(1) where -g/--gnu enables GNU extensions and -G/--traditional disables them. Can we be consistent about that? Do we have other programs with gnu-compat mode? -uwe
Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
On Wed, Feb 17, 2021 at 17:49:15 -, Christos Zoulas wrote: > In article , > Valery Ushakov wrote: > > >But to get back to my main point, PLEASE, can we stop making random > >aimless changes without prior discussion? > > Here's the change I'd like to make: > - pass the alignment instead of the mask (as Roy asked and to match the > other macro) > - use alignof to determine that alignment and CTASSERT what we expect > - remove unused macros > > This incrementally improves things. [...] > diff -u -p -u -r1.688 param.h > --- sys/param.h 15 Feb 2021 19:46:53 - 1.688 > +++ sys/param.h 17 Feb 2021 17:45:55 - > @@ -290,7 +290,7 @@ > #ifdef __NO_STRICT_ALIGNMENT > #define POINTER_ALIGNED_P(p, a) 1 > #else > -#define POINTER_ALIGNED_P(p, a) (((uintptr_t)(p) & (a)) == 0) > +#define POINTER_ALIGNED_P(p, a) (((uintptr_t)(p) & ((a) - 1)) > == 0) > #endif > > /* This incrementally improves wrong things b/c you still have the "is aligned" and "ought to be aligned" conflated in one. -uwe
Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
On Tue, Feb 16, 2021 at 18:51:43 -0500, Christos Zoulas wrote:
> On Feb 17, 2:20am, u...@stderr.spb.ru (Valery Ushakov) wrote:
> -- Subject: Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
>
> | Well, it was you who did the definion of ALIGNED_POINTER centralized
> | and overridable :)
> |
> | revision 1.400
> | date: 2012-01-25 00:03:36 +0400; author: christos; state: Exp; lines:
> +26 -1;
> | Use and define ALIGN() ALIGN_POINTER() and STACK_ALIGN() consistently,
> | and avoid definining them in 10 different places if not needed.
>
> If you look a bit deeper into that change, it merged many MD copies
> of this macro into one, and I needed the override for the archs that
> were different.
>
> | ALIGNED_POINTER is overriden on x86 to be always true. Surprisingly
> | it is not overriden for m68k and vax that are __NO_STRICT_ALIGNMENT.
> | That is most likely an oversight, but that will probably require some
> | cvs archaeology to confirm. Some uses of ALIGNED_POINTER are inside
> | an __NO_STRICT_ALIGNMENT #ifdef.
>
> This is a mess as you can see. The problem is that in each case we
> need to determine if this macro is used to test alignment to determine
> access restrictions on certain architectures (most cases), or it
> is done for performance/protocol requirements.
>
> I hope that nothing falls into the second category and then we can
> use a single macro that uses a combination of __NO_STRICT_ALIGNMENT
> and __alignof() which my guess is that it did not exist at the time
> the macro was invented, and thus it used sizeof() and limited it to
> integral types.
>
> | We don't even seem to be sure about its semantics, as far as I can
> | tell (see bus space comments in my mail).
> |
> | That's even more of a reason to stop doing aimless random changes
> | without getting some kind of understanding first. The last thing we
> | need is ALIGNED_POINTER and POINTER_ALIGNED macros with slighly
> | different semantics both of which are counter-intuitive to begin with
> | (and riastradh@ even had to add a verbose comment for that).
>
> This change was not aimless. I wanted to remove the multiple copies of
> the m_copyup/m_pullup code into one function. To do that I needed the
> alignment as a value, not a predicate macro (that was again copied in
> ~10 places). When I tried to centralize it, I wanted to do the minimal
> change so I would not screw it up (and I did end up screwing it up).
>
> This is a good opportunity now to clean this up even more and provide
> sane alignment macros.
We should start by at least separating the concerns. The test for
"aligned at power of two boundary" and the actual intent/purpose of
that test ("stuff needs to be aligned for us to safely do $FOO").
Then we can test the alignment check without jumping through
ridiculous hoops. That should have been done earlier for the
ALIGNED_POINTER change, but yeah, I can see how in the heat of the
moment that change was just "preserve the status quo" and that's
absolutely fine. But doing the same thing now with the alignment test
and the purpose of that alignment test conflacted once again under a
different but confusignly similar name is negligent (if anything we
will run out of half way decent names for the just-the-alignment-test
macro, b/c all of them will be loaded with some additional accidental
semantic).
-uwe
Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
On Wed, Feb 17, 2021 at 00:51:03 +0100, Roland Illig wrote: > 17.02.2021 00:25:10 Valery Ushakov : > > On Tue, Feb 16, 2021 at 23:54:46 +0100, Roland Illig wrote: > >> Yes, it does. That's what the "#undef __NO_STRICT_ALIGNMENT" in the > >> test is for. > >> > >> I intentionally placed it between (which defines that > >> macro on x86 and some other platforms) and (which uses the > >> macro to switch between the boring "everything is correctly aligned" and > >> the more interesting formula suggested here. > > > > This is wrong on so many levels. What is even the point of a test > > that doesn't test the thing as it is actually defined and used in the > > code? > > The point of the test is to verify that the "complicated" formula > produces correct results. That's what several commits tried to > fix. If this test had been there from the beginning, none of the > wrong formulas would have passed it. That's the whole point. > > The point of the test was intentionally not to test the actual > behavior on each platform but to test the same formula, independent > from the platform, and to do this, I somehow needed access to that > formula. Testing the actually used formula per platform could be > added as another test. I just wanted to avoid the obviously wrong > formulas to go unnoticed in the code. That's the point of the test, > and that's exactly what it achieves. Therefore I don't see anything > wrong with it. The very fact that you need to undefine an unspecified macro at an unspecified time to get to the "formula" points to a problem. We shouldn't be pretending that it's not, and provide the false decorum of "oh, but it's covered with a test, so it's ok". -uwe
Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
On Tue, Feb 16, 2021 at 17:53:09 -0500, Christos Zoulas wrote: > In this case "type" is a struct and we have __alignof() to handle > it, but does this give the right answer? > > Also ALIGNED_POINTER is not conditional to __NO_STRICT_ALIGNMENT and > can be overriden (the opposite goes for POINTER_ALIGNED_P) I am all > for having one macro, but how can we satisfy all the different > semantics? Well, it was you who did the definion of ALIGNED_POINTER centralized and overridable :) revision 1.400 date: 2012-01-25 00:03:36 +0400; author: christos; state: Exp; lines: +26 -1; Use and define ALIGN() ALIGN_POINTER() and STACK_ALIGN() consistently, and avoid definining them in 10 different places if not needed. ALIGNED_POINTER is overriden on x86 to be always true. Surprisingly it is not overriden for m68k and vax that are __NO_STRICT_ALIGNMENT. That is most likely an oversight, but that will probably require some cvs archaeology to confirm. Some uses of ALIGNED_POINTER are inside an __NO_STRICT_ALIGNMENT #ifdef. We don't even seem to be sure about its semantics, as far as I can tell (see bus space comments in my mail). That's even more of a reason to stop doing aimless random changes without getting some kind of understanding first. The last thing we need is ALIGNED_POINTER and POINTER_ALIGNED macros with slighly different semantics both of which are counter-intuitive to begin with (and riastradh@ even had to add a verbose comment for that). -uwe
Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
On Tue, Feb 16, 2021 at 23:54:46 +0100, Roland Illig wrote:
> On 16.02.2021 23:40, Valery Ushakov wrote:
> > On Tue, Feb 16, 2021 at 22:56:19 +0100, Roland Illig wrote:
> >
> > > On 16.02.2021 19:46, Roy Marples wrote:
> > > > I suggest we change POINTER_ALIGNED_P to accept the alignment value we
> > > > want rather than the bitwise test we supply, like so:
> > > >
> > > > #define POINTER_ALIGNED_P(p, a) (((uintptr_t)(p) & ((a) - 1)))
> > >
> > > To make sure that this macro doesn't get broken again, I have written
> > > the attached tests. I will commit them after making sure I got the
> > > changes to distrib/sets/lists/tests/mi correct. All the rest I already
> > > tested successfully.
> >
> > I don't see any proposal to change the meaning of this macro to
> > actually require the alignment even for arches without strict
> > alignment. Does the attached test really passes on e.g. x86 where the
> > macro is always true? E.g. this one:
> >
> > > + if (POINTER_ALIGNED_P(p + 1, 2))
> > > + atf_tc_fail("p + 1 must not be aligned to 2");
>
> Yes, it does. That's what the "#undef __NO_STRICT_ALIGNMENT" in the
> test is for.
>
> I intentionally placed it between (which defines that
> macro on x86 and some other platforms) and (which uses the
> macro to switch between the boring "everything is correctly aligned" and
> the more interesting formula suggested here.
This is wrong on so many levels. What is even the point of a test
that doesn't test the thing as it is actually defined and used in the
code?
-uwe
Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
On Tue, Feb 16, 2021 at 22:56:19 +0100, Roland Illig wrote:
> On 16.02.2021 19:46, Roy Marples wrote:
> > I suggest we change POINTER_ALIGNED_P to accept the alignment value we
> > want rather than the bitwise test we supply, like so:
> >
> > #define POINTER_ALIGNED_P(p, a) (((uintptr_t)(p) & ((a) - 1)))
>
> That's a good definition, clear, simple, obvious, without any surprises.
Now, replace the value "a" with the type "t" and change (a) to
sizeof(t) and you will get ALIGNED_POINTER from just above.
> To make sure that this macro doesn't get broken again, I have written
> the attached tests. I will commit them after making sure I got the
> changes to distrib/sets/lists/tests/mi correct. All the rest I already
> tested successfully.
I don't see any proposal to change the meaning of this macro to
actually require the alignment even for arches without strict
alignment. Does the attached test really passes on e.g. x86 where the
macro is always true? E.g. this one:
> + if (POINTER_ALIGNED_P(p + 1, 2))
> + atf_tc_fail("p + 1 must not be aligned to 2");
> Is my assumption correct that on each platform supported by NetBSD, a
> variable of type double gets aligned to a multiple of 8, even on the
> stack?
No. E.g. on sh3 doubles are 8 bytes but are 4 bytes aligned. I'm
almost sure some other ABI has that kind less strict alignment too,
but I don't remember.
> I wanted to keep the test as simple as possible, therefore I
> didn't want to call malloc just to get an aligned pointer.
You can use an ordinary array that is large enough and manually
find/construct a suitably aligned pointer value inside that array.
BUT, can we, PLEASE, stop making random breaking changes and at least
articulate first what is that that we want here?
POINTER_ALIGNED_P should have never been brought into existence in the
first place.
ALIGNED_POINTER seems to be used to define BUS_SPACE_ALIGNED_POINTER -
the real fun here is sys/arch/x86/include/bus_defs.h that defines
BUS_SPACE_ALIGNED_POINTER to be "really" aligned for BUS_SPACE_DEBUG,
which seems kinda suspicious to me. BTW, can we really conclude from
the CPU's alignment requirements to some bus alignment requirements?
But to get back to my main point, PLEASE, can we stop making random
aimless changes without prior discussion?
To quote Vonnegut, "If I had of been a observer, I would of said we
was comical."
-uwe
Re: CVS commit: src
On Sat, Feb 13, 2021 at 14:30:37 +, Roland Illig wrote: > Libcurses can be built in 2 modes: with wide character support or > without (-DDISABLE_WCHAR). The test suite only covers the variant with > wide characters. The single-byte variant has to be tested manually. > Running sysinst with the single-byte libcurses produces the correct > layout. Also, wide char support extends the original curses api, so the test suite that covers the old api plus the new api obviously covers the old api (though I don't think the test suite detects if the curses it tests actually has wide char support, so it might fail to run, but that's a different problem). -uwe
Re: CVS commit: src
Thanks for working on this, but a couple of nitpicks: On Sat, Feb 13, 2021 at 14:30:37 +, Roland Illig wrote: > In sysinst, the installation screen is indented with tabs. Sysinst uses > msgc, which brings its own text layout engine. This engine does not use > addbytes but addch. [...] > This bug largely went unnoticed because most other applications use > addbytes instead, which worked fine all the time. That doesn't make much sense b/c addbytes is not even a stadnard interface. It's an internal low-level function. > It had been introduced somewhere between NetBSD 8.0 and NetBSD 9.0. As I have pointed out, this was introduced in: revision 1.44 date: 2016-11-28 21:25:26 +0300; author: christos; state: Exp; lines: +8 -4; commitid: MHbzU41X9arGoVvz; If we are inserting spaces to account for a tab, move the x position of the cursor, otherwise this is a no-op (Carsten Kunze) Unlike the file's content the commit logs are uneditable, so please, try to make them accurate. curses is a huge mess already as it is (the standard API as such, not (just) our implementation) and etching in stone potentially misleading information doesn't help... -uwe
Re: CVS commit: src/sys/sys
On Wed, Jan 27, 2021 at 01:00:05 +, Jason R Thorpe wrote: > Module Name: src > Committed By: thorpej > Date: Wed Jan 27 01:00:05 UTC 2021 > > Modified Files: > src/sys/sys: device.h > > Log Message: > Define a macro to terminate the common usage of device_compatible_entry > arrays, in order to placate the angry hordes objecting to use of a GCC > extension. It's hardly fair to characterize three people politely inquiring about that choice and pointing out a more standard way to spell what you want to express (that is perfectly in rhyme with the preceding table and is only one character longer too) as "angry hordes objecting". The point is moot anyway, since anonymous unions themselves only officially appeared in C11. -uwe
Re: CVS commit: src/sys/arch
On Mon, Jan 25, 2021 at 20:28:52 +0300, Valery Ushakov wrote:
> On Mon, Jan 25, 2021 at 08:19:44 -0800, Jason Thorpe wrote:
>
> > > On Jan 25, 2021, at 6:22 AM, Kamil Rytarowski wrote:
> > >
> > > I have no problem with this change but I am curious why should we use "{
> > > }"? It's a C GNU extension and C++ syntax.
> >
> > Using { 0 } makes an assumption about the first member of the
> > structure which is not guaranteed to remain true.
>
> The commit message says:
>
> | Since we're using designated initialisers for compat data, we should
> | use a completely empty initializer for the sentinel.
>
> but that "should" is not true. The code that checks that sentinel
> uses some particular member to access it, so, pedantically speaking,
> the initialization must designate that member in the sentinel
> initialization. Yes, this is verbose and doesn't look as pretty, but
> that is what "should" happen here. Using non-standard {} extension
> makes it look nicer, but is not a "should" kind of necessity.
PS: Forgot to add that C++ doesn't have designated initializers (or at
least it didn't last time I looked), so they are in a different
situation here and need an empty initializer list. In C the only
difference it makes is, as far as I can tell, exactly this kind of an
array with a sentinel at the end and the difference is cosmetic.
-uwe
Re: CVS commit: src/sys/arch
On Mon, Jan 25, 2021 at 08:19:44 -0800, Jason Thorpe wrote:
> > On Jan 25, 2021, at 6:22 AM, Kamil Rytarowski wrote:
> >
> > I have no problem with this change but I am curious why should we use "{
> > }"? It's a C GNU extension and C++ syntax.
>
> Using { 0 } makes an assumption about the first member of the
> structure which is not guaranteed to remain true.
The commit message says:
| Since we're using designated initialisers for compat data, we should
| use a completely empty initializer for the sentinel.
but that "should" is not true. The code that checks that sentinel
uses some particular member to access it, so, pedantically speaking,
the initialization must designate that member in the sentinel
initialization. Yes, this is verbose and doesn't look as pretty, but
that is what "should" happen here. Using non-standard {} extension
makes it look nicer, but is not a "should" kind of necessity.
-uwe
Re: CVS commit: src/share/man/man5
On Sat, Jan 16, 2021 at 14:46:46 +1100, matthew green wrote: > > Log Message: > > Tweak wording for consistency: `if empty or not set', not `if unset'. > > is this correct? > > rc complains if unset or not set (ie, if "$foo" is zero length): > > /etc/rc.d/upsdriver: WARNING: $upsdriver is not set properly - see rc.conf(5). What Taylor said. The above warning reflects the status of checkyesnox() that checks that the value is yes/true/1 or no/false/0. -uwe
catman (Was: CVS commit: src/games/fortune/datfiles)
On Sun, Nov 08, 2020 at 17:37:30 +, Kamil Rytarowski wrote: > Module Name: src > Committed By: kamil > Date: Sun Nov 8 17:37:30 UTC 2020 > > Modified Files: > src/games/fortune/datfiles: fortunes > > Log Message: > catman(8) is a past thing Please, revert this. It's completely irrelevant for the joke if netbsd ships catman or not. Also, come to think of it... Removing catman (i.e. user's ability to generate cat pages) is rather different from removing MKCATPAGES, what's going on here? -uwe
Re: CVS commit: src/usr.sbin/puffs/mount_9p
On Sun, Aug 30, 2020 at 17:12:45 -0400, Christos Zoulas wrote: > Module Name: src > Committed By: christos > Date: Sun Aug 30 21:12:45 UTC 2020 > > Modified Files: > src/usr.sbin/puffs/mount_9p: Makefile > > Log Message: > include bsd.init.mk to avoid: > make: Bad conditional expression ` != "no"' in != "no"? -DINET6 : This worked and the :? for was specifically used to so that the ugly early include (required for an ifdef) can be avoided. I don't remember who pointed out this form to me (joerg@ or mrg@ I'd guess). We have more forms like this in the tree, so not much. Is this a recent regression? Is that fallout from make rototill? Are other similar :? use cases broken too? -uwe
Re: CVS commit: src/share/man/man4
On Tue, Aug 25, 2020 at 10:03:28 +0900, Ryo ONODERA wrote: > Valery Ushakov writes: > > > On Mon, Aug 24, 2020 at 19:27:36 +, Ryo ONODERA wrote: > > > >> Module Name: src > >> Committed By: ryoon > >> Date: Mon Aug 24 19:27:36 UTC 2020 > >> > >> Modified Files: > >>src/share/man/man4: viomb.4 > >> > >> Log Message: > >> Add a missing comma > >> > >> And bump date. > > > > I don't think minor edits like this require a date bump. > > > > I'd say, the date bump is needed only for significant changes like > > adding or removing an option, or may be doing a large edit that > > changes the way the information is presented. > > Sorry. > It makes sense. > Should I revert the date changes? Oh, no need. It's not like we are running out of dates :) Thanks. -uwe
Re: CVS commit: src/share/man/man4
On Mon, Aug 24, 2020 at 19:27:36 +, Ryo ONODERA wrote: > Module Name: src > Committed By: ryoon > Date: Mon Aug 24 19:27:36 UTC 2020 > > Modified Files: > src/share/man/man4: viomb.4 > > Log Message: > Add a missing comma > > And bump date. I don't think minor edits like this require a date bump. I'd say, the date bump is needed only for significant changes like adding or removing an option, or may be doing a large edit that changes the way the information is presented. -uwe
Re: CVS commit: src/share/man/man8
On Sat, Aug 15, 2020 at 16:12:40 +0200, Leonardo Taccari wrote: > > +.Ic export > > PKG_PATH=https://ftp.netbsd.org/pub/pkgsrc/packages/NetBSD/9.0/amd64/All > > +.Ic pkg_add pkgin > > [...] > > Can this use cdn.NetBSD.org and made a bit more general instead of > hardcoding version/port please? > > Instead of 9.0/amd64 probably something like: > > $(uname -r | tr -cd '[0-9.]')/$(sysctl -n hw.machine_arch) FWIW, that doesn't fit on the page in the PostScript output (groff -Tps -mandoc) -uwe
Re: CVS commit: src/external/gpl3/gcc/dist/gcc/config/m68k
On Mon, Aug 10, 2020 at 06:24:39 +, Rin Okuyama wrote: > Modified Files: > src/external/gpl3/gcc/dist/gcc/config/m68k: netbsd-elf.h > > Log Message: > PR port-m68k/6 > > Reset STACK_BOUNDARY to default, 16, to fix strange freeze for amiga, > when kernel is compiled by GCC8. This sounds eerily similar to port-macppc/54827 - there's quite a bit of confusion early on on my part there, but scroll to the last couple of mails. http://gnats.netbsd.org/54827 It looks like some logic changed in MI gcc8 that broke netbsd MD config headers and I wonder if you see the same problem here. For macppc the fix was also to undo the netbsd change to STACK_BOUNDARY in favor of PREFERRED_STACK_BOUNDARY. May be we should also check other ports for similar gotcha proactively? -uwe
Re: CVS commit: src/sys/dev/mii
On Tue, Aug 04, 2020 at 07:12:54 +0300, Valery Ushakov wrote: > On Tue, Aug 04, 2020 at 12:50:11 +0900, SAITOH Masanobu wrote: > > > On 2020/08/03 23:00, Valeriy E. Ushakov wrote: > > > Module Name: src > > > Committed By: uwe > > > Date: Mon Aug 3 14:00:41 UTC 2020 > > > > > > Modified Files: > > > src/sys/dev/mii: miidevs_data.h > > > > > > Log Message: > > > mii_knowndevs[] is de facto const, define it as such. > > > > This file is auto-generated by Makefile.miidevs. make -f Makefile.miidevs > > deletes this change. If the change is required, modify Makefile.miidevs. > > Oh, thank you for the heads up. I was really working on something > else and didn't pay attention to the comment that was out off view. I have fixed the devlist2h.awk script that generates them to emit that const. As the generated files come out exactly the same modulo the rcs id (script emits unexpanded one) I think we can pretend I have committed the script change first and then regenerated the miidevs_data.h header :) -uwe
Re: CVS commit: src/sys/dev/mii
On Tue, Aug 04, 2020 at 12:50:11 +0900, SAITOH Masanobu wrote: > On 2020/08/03 23:00, Valeriy E. Ushakov wrote: > > Module Name:src > > Committed By: uwe > > Date: Mon Aug 3 14:00:41 UTC 2020 > > > > Modified Files: > > src/sys/dev/mii: miidevs_data.h > > > > Log Message: > > mii_knowndevs[] is de facto const, define it as such. > > This file is auto-generated by Makefile.miidevs. make -f Makefile.miidevs > deletes this change. If the change is required, modify Makefile.miidevs. Oh, thank you for the heads up. I was really working on something else and didn't pay attention to the comment that was out off view. -uwe
Re: CVS commit: src/usr.bin/printf
On Sun, Jun 28, 2020 at 21:52:23 +0700, Robert Elz wrote: > Date:Fri, 26 Jun 2020 22:05:05 + > From:"Valeriy E. Ushakov" > Message-ID: <20200626220505.e9030f...@cvs.netbsd.org> > > | Modified Files: > | src/usr.bin/printf: printf.1 > | > | Log Message: > | Drop redundant quoting in the nested printf example. > > I don't think that is correct, the quotes around 0x0A were obviously > redundant (and changing it to 0x0a doesn't alter that), but those > around the $(printf ...) are not - without those the result from the > command substitution would be subject to field splitting, and if IFS > happens to contain a \ or an octal digit, bad things will happen. > > In general it is never a good idea to omit quotes around sh word > expansions except in the cases where you actually want field splitting > (or pathname expansion, etc) to happen (and sometimes except in the > contexts where those don't occur anyway, like x=$whatever) > > For the example in question, try running it with IFS=1 or IFS=2 > to see what I mean. Right, but I'd expect people that actually use IFS with octal digits or a backslash to also understand and know how to add necessary quoting in that case. Though perhaps it makes sense to them back for pedagogical purposes. -uwe
Re: CVS commit: src/sys/arch/x86/x86
On Sat, Jun 06, 2020 at 11:25:19 +0200, Kamil Rytarowski wrote: > On 06.06.2020 09:42, Simon Burge wrote: > > "Kamil Rytarowski" wrote: > > > >> Module Name: src > >> Committed By: kamil > >> Date: Fri Jun 5 21:48:04 UTC 2020 > >> > >> Modified Files: > >> > >>src/sys/arch/x86/x86: cpu_rng.c > >> > >> Log Message: > >> > >> Change const unsigned to preprocessor define > >> > >> Fixes GCC -O0 build with the stack protector. > > > > Surely a gcc bug? This almost certainly needs an > > /* XXX gcc stack protector -O0 bug */ comment and > > possibly an entry in doc/HACKS as well otherwise > > someone will come along later and de-uglify this > > change. > > This is not really a GCC bug, as C const is not constexpr. It's > also not the only place with such logic and such workaround. C++ > fixed it and have real const. Doesn't -Wvla help catching these? Should we enable it? -uwe
Re: CVS commit: src/distrib/sets/lists/base
On Tue, Jun 02, 2020 at 19:15:15 +, Roy Marples wrote: > Module Name: src > Committed By: roy > Date: Tue Jun 2 19:15:15 UTC 2020 > > Modified Files: > src/distrib/sets/lists/base: mi > > Log Message: > dhcpcd: delete the obsolete chroot paths > > postinstall will take care of it. Will it? The build only does "fix obsolete" iirc, so if you did a build with old dhcpcd and then did an update build with new, the build will fail with extra files in destdir, I expect. -uwe
Re: CVS commit: src/sys/dev/usb
On Sat, Mar 14, 2020 at 10:27:27 -0400, Christos Zoulas wrote:
> > I don't belive that "if". It's like claiming you got rid of a stain
> > on a wallpaper after you demolish a wall (not load-bearing,
> > fortunately) and have to put it back and put new wallpaper. :) Get rid
> > of the stain, sure; but may be looking closely with a bit of patience
> > might have been less drastic and as effective.
>
> To fix the kernhist ones I looked with a lot of patience and even then,
> I missed quite a few ones (the ones in the final commit). It is really
> difficult to find them, specially because the DPRINTF macros are
> used sometimes for regular debugging and other times for kernhist.
> In the end I had to add a fake printf function in kernhist.h like below,
> and then filter out the error messages about too many arguments for
> format.
>
> christos
>
> --- kernhist.h 2020-03-13 23:03:13.973939910 -0400
> +++ kernhist.h.orig 2020-03-13 22:59:37.237495925 -0400
> @@ -207,6 +207,11 @@
> #define KERNHIST_PRINTNOW(E) /* nothing */
> #endif
>
> +// Just for format checking
> +static __inline __printflike(1, 2) void
> +__kernhist_printf(const char *fmt __unused, ...) {
> +}
> +
> #define KERNHIST_LOG(NAME,FMT,A,B,C,D) \
> do { \
> unsigned int _i_, _j_; \
> @@ -227,6 +232,7 @@
> _e_->v[1] = (uintmax_t)(B); \
> _e_->v[2] = (uintmax_t)(C); \
> _e_->v[3] = (uintmax_t)(D); \
> + __kernhist_printf(FMT, _e_->v[0], _e_->v[1], _e_->v[2], _e_->v[3]); \
> KERNHIST_PRINTNOW(_e_); \
> } while (/*CONSTCOND*/ 0)
How is is affected by the decision to change (or not) 0x%x to %#x?
-uwe
Re: CVS commit: src/sys/dev/usb
On Sat, Mar 14, 2020 at 09:57:36 -0400, Christos Zoulas wrote: > > Even for the ones without the widths specified. E.g. I personally > > prefer zero printed as 0x0, not as 0, so I assume that when people > > choose either one that reflects their preference. Why mess with it? > > It's all so unnecessary. > > Yes, now we are discussing cosmetics (if 0 should be printed as 0 or > 0x0 mostly in debugging messages), since this is the only change > remaining. > > In retrospect, perhaps I should have left it alone, It would have been better to just leave them the hell alone as they are to begin with. This is, I think, the third time in recent memory when people try to "fix" 0x%x -> %#x and each time it goes wrong. We should have learned from that. > but now aside the cosmetics part, we are strictly better off because > all the formats have been fixed Which is true but non sequitur. > (including the 2 ones which we would not have found if I did not > make the %# change). I don't belive that "if". It's like claiming you got rid of a stain on a wallpaper after you demolish a wall (not load-bearing, fortunately) and have to put it back and put new wallpaper. :) Get rid of the stain, sure; but may be looking closely with a bit of patience might have been less drastic and as effective. -uwe
Re: CVS commit: src/sys/dev/usb
On Fri, Mar 13, 2020 at 22:26:05 -0400, Christos Zoulas wrote: > > On Mar 13, 2020, at 10:25 PM, Valery Ushakov wrote: > > > > As I wrote in a follow up email, it changes formatting b/c you didn't > > change field widths and IMO using %# with a field width is mostly > > trouble to begin with. It's not the first time someone tries to do > > this without actually understanding the consequences of the change. > > Please, can we assume that when people write either 0x%x or %#x they > > most likely actaully mean it for whatever reason and that they want > > that specific output format, and it's just rude to change that, > > especially when you do so incorrectly. > > I am reverting the fixed width ones, hold on. Even for the ones without the widths specified. E.g. I personally prefer zero printed as 0x0, not as 0, so I assume that when people choose either one that reflects their preference. Why mess with it? It's all so unnecessary. -uwe
Re: CVS commit: src/sys/dev/usb
On Fri, Mar 13, 2020 at 22:15:31 -0400, Christos Zoulas wrote: > > This was not a part of the PR and is completely cosmetic (surely it > > supports plain %x if it does support %#x). Why was this necessary? > > (I know I would be quite miffed if someone made a change like that to > > my code). > > Yes, that %x formatting change was not part of the PR, but I only > changed 0x%x not plain %x. I did it because as I was fixing the > 0x%x in the log, I started changing them to %#jx so I did it > globally in that directory for consistency. It found two formats > that were 0x%hu... > > So one can view it as a format consistency checker(not just cosmetic). As I wrote in a follow up email, it changes formatting b/c you didn't change field widths and IMO using %# with a field width is mostly trouble to begin with. It's not the first time someone tries to do this without actually understanding the consequences of the change. Please, can we assume that when people write either 0x%x or %#x they most likely actaully mean it for whatever reason and that they want that specific output format, and it's just rude to change that, especially when you do so incorrectly. -uwe
Re: CVS commit: src/sys/dev/usb
On Fri, Mar 13, 2020 at 17:09:14 -0700, Paul Goyette wrote:
> On Sat, 14 Mar 2020, Valery Ushakov wrote:
>
> > On Fri, Mar 13, 2020 at 14:17:42 -0400, Christos Zoulas wrote:
> >
> > > Log Message:
> > > PR/55068: sc.dying: Fix printf formats:
> > [...]
> > > - 0x% -> %#
> >
> > This was not a part of the PR and is completely cosmetic (surely it
> > supports plain %x if it does support %#x). Why was this necessary?
> > (I know I would be quite miffed if someone made a change like that to
> > my code).
>
> Plain %x - no :(
>
> In order to enable sysctl-transport to userland, all the args need to
> be promoted to %jx, and the format strings need to ensure that they
> consume that size.
Random sample from the diff:
- printf("%s: expect 0xe6!! (0x%x)\n", device_xname(sc->sc_dev),
+ printf("%s: expect 0xe6!! (%#x)\n", device_xname(sc->sc_dev),
Actually, looking close I see diffs like
- DPRINTFN(MD_ROOT, "wValue=0x%04jx", value, 0, 0, 0);
+ DPRINTFN(MD_ROOT, "wValue=%#04jx", value, 0, 0, 0);
that are plain wrong as %#x counts the 0x prefix towards the field
width.
$ printf '0x%02x %#02x\n' 1 1
0x01 0x1
$ printf '0x%08x 0x%08x\n%#08x %#08x\n' 0 1 0 1
0x 0x0001
0x01
So, as far as I can tell, these %# changes don't fix all the argument
size issues, break some output formatting and violate preference of
the original author. Did I miss something else?
-uwe
Re: CVS commit: src/sys/dev/usb
On Fri, Mar 13, 2020 at 14:17:42 -0400, Christos Zoulas wrote: > Log Message: > PR/55068: sc.dying: Fix printf formats: [...] > - 0x% -> %# This was not a part of the PR and is completely cosmetic (surely it supports plain %x if it does support %#x). Why was this necessary? (I know I would be quite miffed if someone made a change like that to my code). -uwe
Re: CVS commit: src/lib/libcurses
On Wed, Mar 11, 2020 at 23:47:40 +, Roy Marples wrote: > Module Name: src > Committed By: roy > Date: Wed Mar 11 23:47:40 UTC 2020 > > Modified Files: > src/lib/libcurses: erase.c > > Log Message: > curses: Fix werase(3) wide character support > > We need to consider erasing all attributes, not just WA_ATTRIBUTES. > Fixes PR lib/23910. > > While here, make the function a little more readable. Do we need the same change in wclrtoeol()? -uwe
Re: CVS commit: src/sys/conf
On Sat, Mar 07, 2020 at 19:18:41 -0500, Christos Zoulas wrote: > Module Name: src > Committed By: christos > Date: Sun Mar 8 00:18:41 UTC 2020 > > Modified Files: > src/sys/conf: files > > Log Message: > undo previous since config has been fixed It's still not. Nested ifdefs are not handled correctly. -uwe
Re: CVS commit: src/external/bsd/tcpdump/dist
On Mon, Feb 24, 2020 at 16:19:35 +, Kamil Rytarowski wrote: > Module Name: src > Committed By: kamil > Date: Mon Feb 24 16:19:35 UTC 2020 > > Modified Files: > src/external/bsd/tcpdump/dist: extract.h > > Log Message: > Rearrange the code to make UNALIGNED_OK available for __NetBSD__ s/unsigned/undefined/ in the comment We have __GNUC_PREREQ__() for version tests -uwe
Re: CVS commit: src/lib/libc/stdlib
On Sun, Feb 23, 2020 at 10:57:28 +0100, Kamil Rytarowski wrote: > On 23.02.2020 08:46, Martin Husemann wrote: > > > Source code consistency is a very important stylistic plus, every break of > > that should be accompanied by a comment. > > Done. Thank you. -uwe
Re: CVS commit: src/lib/libc/stdlib
On Sun, Feb 23, 2020 at 03:35:19 +0100, Kamil Rytarowski wrote: > On 23.02.2020 03:20, Valery Ushakov wrote: > > On Sun, Feb 23, 2020 at 02:51:49 +0100, Kamil Rytarowski wrote: > > > >> On 23.02.2020 02:29, Valery Ushakov wrote: > >>> On Sat, Feb 22, 2020 at 14:07:57 +, Kamil Rytarowski wrote: > >>> > >>>> Module Name: src > >>>> Committed By:kamil > >>>> Date:Sat Feb 22 14:07:57 UTC 2020 > >>>> > >>>> Modified Files: > >>>> src/lib/libc/stdlib: _rand48.c > >>>> > >>>> Log Message: > >>>> Avoid undefined behavior in the rand48(3) implementation > >>>> > >>>> Instead of implicid promotion to signed int, > >>>> explicitly cast the arguments to unsigned int. > >>> > >>> Please, please, please, pay at least some attention to what is going > >>> on around the code you are changing. > >>> > >>> If there's already code in this function that does: > >>> > >>>accu = (unsigned long) __rand48_mult[0] * (unsigned long) xseed[0]; > >>> > >>> then keep it consistent and don't do casts to a different type > >>> > >>>accu += (unsigned int) __rand48_mult[0] * (unsigned int) xseed[2]; > >> > >> cast to unsigned long still works, but changes algorithm. My change was > >> performed deliberately. On the other hand and according to local tests > >> the end-result for unsigned long produces the same reults as cast to > >> unsigned int and unsigned char so it does not matter. > > > > I cannot make sense of your answer. Does the cast to unsigned long > > there change the algorithm or does it produce the same result? If it > > produces the same result, then it should be used to be consistent with > > the rest of the code (or the rest of the code changed to use unsigned > > int). If it does change the result, there should be a comment > > explaining it. > > Algorithm would be changed from calculating on 32bit numbers with signed > integer overflows to an algorithm calculating on 64bit numbers. The > __dorand48() function truncates the result to least significant 16bits > only so it does not matter. I retained operations on 32bits avoiding > changes of types for stylistic reasons. That still doesn't make sense to me. You took your time to figure out whats going on in this bit of code. Then you make a change that looks extremely unobvious b/c it is inconsistent with the rest of the code, and you say you did this for stylistic reasons. The next person looking at that code (in $bignum years) will have to waste their time puzzling out the reason. Why not use the knowledge you've gained of this code for good and change the code properly? The 90s unsigned long was probably meant to be 32-bit anyway (cf. X11 mess up with using long for 32 bit quantities in the protocol and then running into issues when 64-bit happened). So if doing things in 32-bit here is the right and intended thing to do, then change that unsigned long to uint32_t. If you don't want to dive that deep (which is entirely understandable), then, exactly for stylistic reasons, cast to unsigned long to be consistent with the old code - as you already established that it didn't change the result. -uwe
Re: CVS commit: src/lib/libc/stdlib
On Sun, Feb 23, 2020 at 02:51:49 +0100, Kamil Rytarowski wrote: > On 23.02.2020 02:29, Valery Ushakov wrote: > > On Sat, Feb 22, 2020 at 14:07:57 +, Kamil Rytarowski wrote: > > > >> Module Name: src > >> Committed By: kamil > >> Date: Sat Feb 22 14:07:57 UTC 2020 > >> > >> Modified Files: > >>src/lib/libc/stdlib: _rand48.c > >> > >> Log Message: > >> Avoid undefined behavior in the rand48(3) implementation > >> > >> Instead of implicid promotion to signed int, > >> explicitly cast the arguments to unsigned int. > > > > Please, please, please, pay at least some attention to what is going > > on around the code you are changing. > > > > If there's already code in this function that does: > > > >accu = (unsigned long) __rand48_mult[0] * (unsigned long) xseed[0]; > > > > then keep it consistent and don't do casts to a different type > > > >accu += (unsigned int) __rand48_mult[0] * (unsigned int) xseed[2]; > > cast to unsigned long still works, but changes algorithm. My change was > performed deliberately. On the other hand and according to local tests > the end-result for unsigned long produces the same reults as cast to > unsigned int and unsigned char so it does not matter. I cannot make sense of your answer. Does the cast to unsigned long there change the algorithm or does it produce the same result? If it produces the same result, then it should be used to be consistent with the rest of the code (or the rest of the code changed to use unsigned int). If it does change the result, there should be a comment explaining it. -uwe
Re: CVS commit: src/lib/libc/stdlib
On Sat, Feb 22, 2020 at 14:07:57 +, Kamil Rytarowski wrote: > Module Name: src > Committed By: kamil > Date: Sat Feb 22 14:07:57 UTC 2020 > > Modified Files: > src/lib/libc/stdlib: _rand48.c > > Log Message: > Avoid undefined behavior in the rand48(3) implementation > > Instead of implicid promotion to signed int, > explicitly cast the arguments to unsigned int. Please, please, please, pay at least some attention to what is going on around the code you are changing. If there's already code in this function that does: accu = (unsigned long) __rand48_mult[0] * (unsigned long) xseed[0]; then keep it consistent and don't do casts to a different type accu += (unsigned int) __rand48_mult[0] * (unsigned int) xseed[2]; -uwe
Re: CVS commit: src/external/apache2/llvm/config/llvm/Config
On Fri, Feb 21, 2020 at 11:13:24 +0100, Kamil Rytarowski wrote: > This patch changes cryptic failure in linking to a verbose message: > > http://netbsd.org/~kamil/patch-00231-llvm-terminfo-tools.txt > > Does it look to commit? The "devel library" is linux-specific terminology. There's no need for ALL CAPS. -uwe
Re: CVS commit: src/external/apache2/llvm/config/llvm/Config
On Thu, Feb 20, 2020 at 00:09:28 +, Roy Marples wrote: > On 19/02/2020 22:29, Kamil Rytarowski wrote: > > Why do you need terminfo/termios in ./build.sh tools? > > We build the nbtic tool so we can build the terminfo database. The database is not a problem. If we want to keep this code turned on in llvm we are better providing a small do-nothing libterminfo stub as a fallback that would claim that the current TERM doesn't support colors. -uwe
Re: CVS commit: src/external/apache2/llvm/config/llvm/Config
On Thu, Feb 20, 2020 at 10:54:50 +0100, Kamil Rytarowski wrote: > >> https://syzkaller.appspot.com/text?tag=CrashLog=11aafc09e0 > > > > Yes and that is completely useless for figuring out the why. > > We don't have any direct reproducer (we tried) and we must figure it > out from syzkaller bot. We don't have access to the machine and a > limited access to an admin over there (who has no expertise on > BSDs). This failure breaks kMSan build for long time now. There is > full build log. We can start by dropping "> /dev/null 2>&1" in tools/llvm/Makefile (may be temporarily). -uwe
Re: CVS commit: src/external/bsd/dhcpcd/dist/src
On Thu, Feb 13, 2020 at 18:25:41 +0100, Kamil Rytarowski wrote: > On 13.02.2020 18:00, Valery Ushakov wrote: > > Arguably, if the tool you use is broken, you shouln't be mutilating > > the source code just to shut that tool up. > > The introduced changes were good, even if GCC would be silent. You changed one occurence just because it happens to trigger a bug in gcc. There are ntohs() >< size_t_var comparisons right above and below the line you changed, where the same promotion happens (without triggering a gcc bug) and they don't have a cast. So someone reading that code will ask themself, wait a minute, why the cast is necessary in *that* place but not in those places? What is going on there that I miss that required introduction of that cast. I.e. your change create cognitive load for the reader. I don't cosider that good. > [...] the promotion rules are considered by many people as the major > flaw in C. Today I prefer explicit casts (after the MUSL lesson) > even if unnecessary than implicit promotion. Amen. However they are there and we made uneasy peace with them so unless you are consistent in your casting, you send all the wrong singals to the reader. > As an alternative option we can disable warnings but this is in my > opinion much worse in this case than potentially overlooking real > problems in a 4000+ line file. I did not propose to disable the warning. I proposed to downgrade -Werror to -Wno-error (i.e. a warning) and only for the buggy sanitizer build. That file will still be compiled in normal builds with all the warnings=errors enabled, so real problems won't be overlooked. > Repeatedly informing that the tool (GCC) is broken did not solve any > issue. It would be finally better to see someone fixing GCC rather > than informing other GCC users about it. Feel free to follow your own advice here... (After all, it's your sanitizer usage that has problems. The normal builds are fine as they are :) > On the opposite side of this if this camp is MUSL. I used chunks of the > MUSL code and it had poor results. There is a strict policy to avoid > casts unless absolutely needed That's not a bad policy. As I said, a cast is a blunt tool. It's easy to introduce a wrong one that would silence some warning or other but will do the wrong thing, but now since there is a cast the compiler cannot even squeak. I might be misremembering, but IIRC Visual C has some warning(s) that ignore explicit casts, precisely to detect that kind of problems. > and if they are needed this tends to be in as obscure way as > possible (like adding U attribute to one of the arguments in an > expression). What's obscure about adding 'U' to signify unsignedness?! Also it's not a cast to begin with, a literal with the 'u' suffix *is* unsigned. A cast is when you take something of one type and coerce it to be of some other type. -uwe
Re: CVS commit: src/external/bsd/dhcpcd/dist/src
On Thu, Feb 13, 2020 at 15:03:35 +0100, Kamil Rytarowski wrote: > On 13.02.2020 14:50, Valery Ushakov wrote: > > On Thu, Feb 13, 2020 at 14:18:43 +0100, Kamil Rytarowski wrote: > > > > Can you show the original problem that you are trying to fix here? > > When and how did you get warning? > > GCC has a property (as called by Joerg bug) that different backend, > different optimization levels etc emit different warnings. > > There is a request from certain people around to fix GCC not to mute > down GCC, only because warnings are emitted in one configuration and not > in their build settings. > > I redirect these complains to GCC upstream. > > dhcpcd emits these warnings once we enable -fsanitize=undefined. If I add -fsanitize=undefined to the command line to compile that file (in current) I still do not see that warning. I didn't have time yet to do a full build with sanitizer enabled. Arguably, if the tool you use is broken, you shouln't be mutilating the source code just to shut that tool up. In general I'm quite worried about the recent ongoing activity of sprinkling changes around the tree just to shut up this or that warning without paying much attention to the end result. Some of those changes were wrong. Some left code looking inconsistent as they fixed one place that did produce a warning, but didn't change nearly identical bits of code just next to it that didn't. That leaves code in a fractured state. Casts in particular are a very blunt instrument, and given that history of recent changes your use of that instrument in this case really raised red flags. If -- and I still haven't seen it myself, "works for me" in a naive test -- -fsanitize=undefined is buggy w.r.t. -Wconversion, then you should arange to use say -Wno-error=conversion for that specific file when -fsanitize=undefined is enabled, we do have makefile machinery for that. This way the bug in the tool is not causing random and dubious changes to the code, and is confined to a properly commented change in a Makefile. -uwe
Re: CVS commit: src/external/bsd/dhcpcd/dist/src
On Thu, Feb 13, 2020 at 14:18:43 +0100, Kamil Rytarowski wrote:
> Feel free to fix GCC.
That's sounds rather dismissive to me.
Can you show the original problem that you are trying to fix here?
When and how did you get warning?
I do NOT see the warning for this kind of conversion with either
current (gcc 8) or on netbsd-8 (gcc 5).
If I revert this change the file is compiled just fine. E.g. with a
very current current build:
$ nbmake-sparc dhcp.o
# compile dhcpcd/dhcp.o
sparc--netbsdelf-gcc -O2 ... -Wconversion -Wsign-compare -c .../dhcp.c
$
> dhcpcd already uses this explicit cast style, e.g. here:
>
> static void *
> get_udp_data(void *packet, size_t *len)
> {
> const struct ip *ip = packet;
> size_t ip_hl = (size_t)ip->ip_hl * 4;
> char *p = packet;
>
> p += ip_hl + sizeof(struct udphdr);
> *len = (size_t)ntohs(ip->ip_len) - sizeof(struct udphdr) - ip_hl;
> return p;
> }
Again, I don't get any warnings if I change that to
... /*(size_t)*/ntohs(ip->ip_len) ...
I also tried
... /*(size_t)ntohs*/(ip->ip_len) ...
as a quick and dirty attempt to emulate little endian where ntohs is a
nop (I don't have an up-to-date little endian build handy) and, again,
there's no warning.
Oh, actually, my tooldir has accreted quite a number of old superh
compilers, so:
$ cat size.c
typedef unsigned short uint16_t;
typedef unsigned long size_t;
#define ntohs(x) (x)
size_t
foo(uint16_t ip_len)
{
return ntohs(ip_len) - sizeof(uint16_t);
}
$ for CC in $TOOLDIR/bin/shle--netbsdelf-gcc-[0-9]*; do echo $(basename $CC);
$CC -Wall -Wextra -Wconversion -Wsign-compare -O2 -S size.c; done
shle--netbsdelf-gcc-4.5.4
shle--netbsdelf-gcc-4.8.3
shle--netbsdelf-gcc-4.8.4
shle--netbsdelf-gcc-4.8.5
shle--netbsdelf-gcc-5.4.0
shle--netbsdelf-gcc-5.5.0
shle--netbsdelf-gcc-6.4.0
shle--netbsdelf-gcc-6.5.0
shle--netbsdelf-gcc-7.4.0
$
-uwe
Re: CVS commit: src/usr.sbin/altq/altqstat
On Fri, Feb 07, 2020 at 20:13:26 +, Santhosh Raju wrote: > Module Name: src > Committed By: fox > Date: Fri Feb 7 20:13:26 UTC 2020 > > Modified Files: > src/usr.sbin/altq/altqstat: quip_client.c > > Log Message: > usr.sbin/altq: Fix -Wstringop-truncation warning. > > Looks like the original intention was to truncate the string at len. > > Replace strncpy(3) with strlcpy(3). > > Error was reported when build.sh was run with MKLIBCSANITIZER=yes flag. > > Reviewed by: kamil@ This looks half-baked. It still uses 63 to cap len (64-1 to accout for strncpy), when the buffer is 64. It wants to cap len precisely b/c it used strncpy. The new strlcpy code should not do it b/c strlcpy doesn't pad the destination. -uwe
sys_ptrace_lwpstatus.c (Was: CVS commit: src/sys)
On Thu, Dec 26, 2019 at 08:52:39 +, Kamil Rytarowski wrote: > Module Name: src > Committed By: kamil > Date: Thu Dec 26 08:52:39 UTC 2019 > > Modified Files: > src/sys/kern: files.kern sys_ptrace_common.c > src/sys/sys: ptrace.h > Added Files: > src/sys/kern: sys_ptrace_lwpstatus.c > > Log Message: > Put ptrace_read_lwpstatus() and process_read_lwpstatus() to a new file > > Fixes "no PTRACE" kernel build, in particular zaurus kernel=INSTALL_C700. This is counterintuitive when a sys_ptrace* file with ptrace_* functions does not depend on options ptrace. That seems to be a strong indication the functions and the file are misnamed. filekern/sys_ptrace.c ptrace filekern/sys_ptrace_common.cptrace filekern/sys_ptrace_lwpstatus.c kern -uwe
Re: CVS commit: src/share/tmac
On Tue, Dec 24, 2019 at 16:13:56 -, Christos Zoulas wrote: > In article <20191224113037.gl24...@pony.stderr.spb.ru>, > Valery Ushakov wrote: > >On Tue, Dec 24, 2019 at 08:46:49 +0700, Robert Elz wrote: > > > >> | |but since we run with -ww we > >> | |get the warning about an unbalanced .el > >> > >> That's a broken warning. > > > >Amen. But let's be honest, in this day and age *very* few people can > >can read troff, never mind write it (and I don't count myself as one), > >so a warning from groff, however broken, will just confuse and upset > >the users. Since silencing this warning doesn't require that much of > >an effort and doesn't mutilate the code that much, it's easier to just > >shut it up. > > We should file a bug report with groff though (and optionally supply a fix). We are sticking to groff 1.19 that is still GPLv2+. 1.20 (released 11 years ago) switched to GPLv3+. Current version is 1.22.4. It would make sense to switch to heirloom doctools (CDDL), but the general sentiment seems to be that we don't want to have troff in base as it's not cool, nobody knows how to use it and we don't use it for manpages anymore anyway. :( For the in-tree stuff we only use troff for isntall notes and for (very very old and often out of date) documents in /usr/share/doc -uwe
Re: CVS commit: src/share/tmac
On Tue, Dec 24, 2019 at 08:46:49 +0700, Robert Elz wrote: > Date:Mon, 23 Dec 2019 23:45:34 +0100 > From:Steffen Nurpmeso > Message-ID: <20191223224534.8ufgy%stef...@sdaoden.eu> > > > | |Troff reads .ie and checks the condition. The condition is true and > | |so the rest of the line is executed. Then troff reads .el that > | |matches successful .ie, so the .el is discarded. > | | > | |Then it reads .el which is not matched with any .ie that troff has > | |seen. It's usually silently discarded, > > That is what is (always was) intended to happen. Sure. > | |but since we run with -ww we > | |get the warning about an unbalanced .el > > That's a broken warning. Amen. But let's be honest, in this day and age *very* few people can can read troff, never mind write it (and I don't count myself as one), so a warning from groff, however broken, will just confuse and upset the users. Since silencing this warning doesn't require that much of an effort and doesn't mutilate the code that much, it's easier to just shut it up. -uwe
Re: CVS commit: src/share/tmac
On Mon, Dec 23, 2019 at 22:13:23 +0100, Steffen Nurpmeso wrote: > Valeriy E. Ushakov wrote in <20191223201734.0e415f...@cvs.netbsd.org>: > > |Modified Files: > | src/share/tmac: doc2html > | > |Log Message: > |Fix if/else syntax in previous. > > What was wrong with that? > > . ie 'utf8'\*[.T]' \ > . ds mx:istty > . el .ie 'latin1'\*[.T]' \ > . ds mx:istty > . el .ie 'ascii'\*[.T]' \ > . ds mx:istty > . el .ie 'pdf'\*[.T]' \ > . mx:dump-init-pdf > . el .ie 'html'\*[.T]' \ > . mx:dump-init-html > . el \ > . rm mx:gogogo > > is perfectly valid roff code. Try running it with warnings (-ww), as the build process does. You'll get warning: unbalanced .el request Consider .ie '\*(.T'ascii' .ds html-charset US-ASCII .el .ie '\*(.T'latin1' .ds html-charset ISO-8859-1 .el .ie '\*(.T'utf8' .ds html-charset UTF-8 .el .ab unsupported encoding \*(.T executed with -Tascii. Troff reads .ie and checks the condition. The condition is true and so the rest of the line is executed. Then troff reads .el that matches successful .ie, so the .el is discarded. Then it reads .el which is not matched with any .ie that troff has seen. It's usually silently discarded, but since we run with -ww we get the warning about an unbalanced .el The the same happens again for the next .el, the number of warnings depends on which .ie in the chain was successful (no warnings for utf-8, 1 warning for latin1, two for ascii). -uwe
Re: CVS commit: src/share/man/man9
On Sat, Dec 07, 2019 at 12:22:19 +, Thomas Klausner wrote: > Modified Files: > src/share/man/man9: atomic_loadstore.9 > > Log Message: > Simplify macro usage. > > > To generate a diff of this commit: > cvs rdiff -u -r1.3 -r1.4 src/share/man/man9/atomic_loadstore.9 This breaks formatting, adding spaces between function names and the opening parens - though, please, don't fix that, the original markup is way overcomplicated anyway, I'll work with riastradh@ on improvments. -uwe
Re: CVS commit: src/distrib/notes/common
On Wed, Dec 04, 2019 at 19:59:06 +0700, Robert Elz wrote: > While "Administrivia" is not really a word, it is a common > slang expression indicating trivial issues that are related > to administration (I have no idea what Administrativa might be, > "administrative" is a word, but meaningless in that context). It now made its way into the real dictionaries too https://www.merriam-webster.com/dictionary/administrivia -uwe
Re: CVS commit: src/distrib/notes/common
My apologies. Thanks for the heads up. I was running the extraction script on src only. I will restore these entries. On Tue, Dec 03, 2019 at 08:00:40 +0900, Izumi Tsutsui wrote: > Date: Tue, 3 Dec 2019 08:00:40 +0900 > From: Izumi Tsutsui > Subject: Re: CVS commit: src/distrib/notes/common > To: source-changes-d@NetBSD.org > Cc: u...@netbsd.org, tsut...@ceres.dti.ne.jp > > > Module Name:src > > Committed By: uwe > > Date: Mon Dec 2 20:57:17 UTC 2019 > > > > Modified Files: > > src/distrib/notes/common: legal.common > > > > Log Message: > > Drop the entry for Yasushi Yamasaki. Nothing in the tree requires it. > > X68k Xserver sources do: > > http://cvsweb.netbsd.org/bsdweb.cgi/xsrc/external/mit/xorg-server/dist/hw/netbsd/x68k/x68kFb.c?rev=1.2=text/x-cvsweb-markup > > --- > Izumi Tsutsui On Tue, Dec 03, 2019 at 08:05:58 +0900, Izumi Tsutsui wrote: > Date: Tue, 3 Dec 2019 08:05:58 +0900 > From: Izumi Tsutsui > Subject: Re: CVS commit: src/distrib/notes/common > To: source-changes-d@NetBSD.org > Cc: u...@netbsd.org, tsut...@ceres.dti.ne.jp > > > Module Name:src > > Committed By: uwe > > Date: Mon Dec 2 17:28:36 UTC 2019 > > > > Modified Files: > > src/distrib/notes/common: legal.common > > > > Log Message: > > Drop entries for "K. Kobayashi" and "K. Kobayashi and H. Shimokawa" > > that are no longer referenced in tree. > > http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/dev/ieee1394/fwohci.c?rev=1.144=text/x-cvsweb-markup > etc. > > --- > Izumi Tsutsui -uwe
Re: CVS commit: src/sys/kern
On Thu, Nov 07, 2019 at 19:06:29 +0100, Martin Husemann wrote: > OK, why is it 8 byte aligned? Checking > > > revision 1.108 > > date: 2011-01-18 20:52:24 +0100; author: matt; state: Exp; lines: +2 -1; > > Make struct disklabel 8 byte aligned. This increases its size by 4 bytes > > on IPL32 platforms so add code in sys_ioctl (and netbsd32_ioctl) to deal > > with the older/smaller diskabel size. This change makes disklabel the > > same for both IPL32 and LP64 platforms. > > Argh! Somehow I dimly remember having been here already ... > I think I proposed to change the loop to 8 byte increments back then, but > noone knows what odd systems it will the stop working on (none at all is my > personal bet). I gather the alignment mess is caused by pointers d_un.un_b.un_d_boot0 and d_un.un_b.un_d_boot1 that are inside the d_un union and are NOT part of the actual disklabel: "These are returned when using getdiskbyname(3) to retrieve the values from /etc/disktab." So that was completely self-inflicted. The loop is fine for the on-disk structure. Now the question is how to dig our way out of this, b/c unfortunately it's a public interface. But the mindless memcpy should be the last resort IMO. -uwe
Re: CVS commit: src/sys/kern
On Thu, Nov 07, 2019 at 18:08:40 +0100, Kamil Rytarowski wrote: > On 07.11.2019 16:45, Kamil Rytarowski wrote: > > On 07.11.2019 16:26, Martin Husemann wrote: > >> On Thu, Nov 07, 2019 at 02:53:08PM +0100, Kamil Rytarowski wrote: > >>> On 07.11.2019 14:25, Valery Ushakov wrote: > >>>> If the sanitizer does complain about other uses, there is little point > >>>> in fixing one instance and not the others. > >>> > >>> We already agreed with Christos that this is appeasing of GCC. If you > >>> want to scan the whole kernel (or whole C) file for more occurrences of > >>> violations - please go for it. > >> > >> No. The commit needs to be reverted, and then > >> > >> a) either the root cause for the unaligned address be fixed or > >> b) some other means be found to make the sanitizer shut up > >> > >> As uwe said: papering over a tiny detail that *never* hits in the real > >> world but potentialy hiding a real issue is not the way to go. > >> > > > > I don't have a readily available reproducer locally but it was breaking > > syzbot from booting after the switch to gcc8. I will fix it differently > > aligning the whole struct (so the same approach as we use in userland) > > and backout this change. > > > > Please review: > > http://netbsd.org/~kamil/patch-00194-disklabel-alignment.txt > > This patch works for me. What happens if you change check_label_magic() to use direct member accesses (as the code did before xtos change it) instead of memcmp? Does that shup up the sanitizer? I assume it should as it doesn't complain about other member accesses. I'd strongly prefer this change for now. -uwe
Re: CVS commit: src/sys/kern
On Thu, Nov 07, 2019 at 09:58:06 -0600, David Young wrote: > On Thu, Nov 07, 2019 at 04:26:51PM +0100, Martin Husemann wrote: > > On Thu, Nov 07, 2019 at 02:53:08PM +0100, Kamil Rytarowski wrote: > > > On 07.11.2019 14:25, Valery Ushakov wrote: > > > > If the sanitizer does complain about other uses, there is little point > > > > in fixing one instance and not the others. > > > > > > We already agreed with Christos that this is appeasing of GCC. If you > > > want to scan the whole kernel (or whole C) file for more occurrences of > > > violations - please go for it. > > > > No. The commit needs to be reverted, and then > > > > a) either the root cause for the unaligned address be fixed or > > b) some other means be found to make the sanitizer shut up > > > > As uwe said: papering over a tiny detail that *never* hits in the real > > world but potentialy hiding a real issue is not the way to go. > > > > Martin > > > > P.S.: Independend of this I would still like an official C standard > > clarification; in my reading a simple address calculation is not > > accessing an object through a pointer (which would be the undefined > > behaviour). If the C standard is not clear on this, it needs to be > > improved. > > I think the problem is that if you have the series of statements, > > element_t *e = >element; > > if (s == NULL) > return; > > then the comparison with NULL implies that in this scope, s could > be NULL. NULL does not necessarily have any "arithmetic" relationship > to any other pointer---by that rationale, you probably cannot assign > any alignment to it---so there is no sensible value that you can > give to e. This is not what the changed code does. The code in question has struct disklabel *dlp = ...; apparently gcc complains about memcmp(>d_magic, ...) but later the code uses e.g. dlp->d_partitions (right after the check_label_magic call) and other memebers. So it's very suspicious that one usage is flagged and others are not. Until very recently the magic check was also explicitly comparing dlp->d_magic != DISKMAGIC, etc. So may be we should stop pretending and rewrite check_label_magic() to use that instead of memcmp. (And then fix all dlp->foo in one swoop). If my I interpretation is wrong, I would be glad to be corrected. > There is probably an argument to be made that in a > segmented/tagged/capability architecture that has run C programs > (8086; Burroughs Large Systems) or may run them in the future (CHERI), > &(NULL)->element cannot sensibly be computed. Amen :). I actually did encounter problems like that when compiling software on Xenix 286 ages ago (e.g. 0 instead of NULL passed as the last argument to exec). While that is a fascinating excercise, it's not what happens here. -uwe
Re: CVS commit: src/sys/kern
On Thu, Nov 07, 2019 at 15:48:55 +0300, Valery Ushakov wrote: > On Thu, Nov 07, 2019 at 13:37:21 +0100, Kamil Rytarowski wrote: > > > On 07.11.2019 13:17, Valery Ushakov wrote: > > > On Thu, Nov 07, 2019 at 06:02:39 +0100, Kamil Rytarowski wrote: > > > > > >> I have checked received the following patch and received a feedback from > > >> a LLVM developer. > > >> > > >> On 07.11.2019 05:47, 'Dmitry Vyukov' via syzkaller-netbsd-bugs wrote: > > >>> I've consulted with some people and _presumably_ (to the degree one > > >>> can be sure about bitter corner cases of C/C++ :)) this is a correct > > >>> fix (and formally correct warnings from ubsan). > > >>> As 6.5.3.2/4 says, only &*p and [i] syntactic forms are defined as > > >>> special case of not being a dereference, but rather effectively an > > >>> address calculation. But >m is not. Thus it is interpreted as a > > >>> dereference that produces an lvalue and then taking address of that > > >>> lvalue. At the point of dereference we have UB. Your fix avoids the > > >>> dereference. > > > > > > The context is lost in the thread, but the original change was about > > > >d_magic as far as I can figure out. If the claim is that that's > > > UB b/c dlp is improperly aligned, then why the half of the rest of the > > > file is not UB as it uses the same "dlp" pointer to access other > > > members of the disklabel. > > > > We were already addressing various reports for disklabel related code in > > the kernel and userland. In userland we as far as I recall just copy the > > struct into an aligned promptly pointer. > > > > There might be more problems, but we address them as they pop up. > > That seems counterintuitive. There's the root cause and when/if that > root cause is fixed, then this particular problem will be fixed as > well. The concern obviously is that when the root problem is fixed, > this change will be forgotten and the unnecessarily uglified code will > be just left over as it currently is. > > So the change is extremely questionable from that point of view. It > creates the illusion of things being "fixed" while in the longer run > I'd say it harms the code. And as I said, if the sanitizer flags that > place as UB and doesn't flag dozens of dlp->foo accesses elsewhere in > that file, then either I don't understand what the sanitizer complains > about, or it's not a very good sanitizer and we shouldn't care to shut > it up in this random place that triggers it. Lest the legalese part of this sub-thread distract from this point let me re-iterate. I'm not questioning primarily whether that >d_magic is UB or not (it most likely is). What irks me about this change is that we tweak one random instance of a larger problem and disregard the rest of it. If I misunderstand the problem and other uses of dpl->foo in the same file are ok, please, kindly point this out to me. If the other uses are indeed problematic, then does or doesn't the sanitizer complain about them, like it complains about that check that was "fixed" in the original commit? If the sanitizer does complain about other uses, there is little point in fixing one instance and not the others. If the sanitizer does NOT complain about other uses, then please find a different way to shup the stupid thing up. (Which is also how I read the responses from Martin and Christos, but that's just my interpretation). -uwe
Re: CVS commit: src/sys/kern
On Thu, Nov 07, 2019 at 13:59:37 +0100, Kamil Rytarowski wrote: > On 07.11.2019 13:48, Valery Ushakov wrote: > > On Thu, Nov 07, 2019 at 13:37:21 +0100, Kamil Rytarowski wrote: > > > >> On 07.11.2019 13:17, Valery Ushakov wrote: > >>> On Thu, Nov 07, 2019 at 06:02:39 +0100, Kamil Rytarowski wrote: > >>> As a side note - the C99 standard contains "derefer" exactly once, in > >>> a footnote. Since we have ended up in the darkest corners of > >>> legalistic exegesis, please, can we avoid using the word that is, > >>> technically speaking, meaningless as far as this discussion is > >>> concerned? > >> > >> Unary * oprator. C++ specified term "dereferenceable" in the context of > >> the unary * operator. > > > > This is C code and the C standard is hard enough as it is already. > > Please, can we put the C++ aside for a moment? > > No. The kernel was already patched (years ago) to build as a valid C++ > software. "No" what? This is C code. If it also happens to be a valid C++ code, good for it, but that is a separate matter. There's a claim made about that code that it triggers UB according to the C standard. That claim can be meaningfully dicussed in that legal(ese) framework only. Only after the meaning of the competing claims about that code is clarified within its "native" framework can we consider C++ interpretation of the same code as a followup and whatever interplay there is between the standards. So, if that code is supposed to be valid C code *and* valid C++ code, then it should be at least valid C code and so, please, can we for now stick to that part of the "and"? -uwe
Re: CVS commit: src/sys/kern
On Thu, Nov 07, 2019 at 13:37:21 +0100, Kamil Rytarowski wrote: > On 07.11.2019 13:17, Valery Ushakov wrote: > > On Thu, Nov 07, 2019 at 06:02:39 +0100, Kamil Rytarowski wrote: > > > >> I have checked received the following patch and received a feedback from > >> a LLVM developer. > >> > >> On 07.11.2019 05:47, 'Dmitry Vyukov' via syzkaller-netbsd-bugs wrote: > >>> I've consulted with some people and _presumably_ (to the degree one > >>> can be sure about bitter corner cases of C/C++ :)) this is a correct > >>> fix (and formally correct warnings from ubsan). > >>> As 6.5.3.2/4 says, only &*p and [i] syntactic forms are defined as > >>> special case of not being a dereference, but rather effectively an > >>> address calculation. But >m is not. Thus it is interpreted as a > >>> dereference that produces an lvalue and then taking address of that > >>> lvalue. At the point of dereference we have UB. Your fix avoids the > >>> dereference. > > > > The context is lost in the thread, but the original change was about > > >d_magic as far as I can figure out. If the claim is that that's > > UB b/c dlp is improperly aligned, then why the half of the rest of the > > file is not UB as it uses the same "dlp" pointer to access other > > members of the disklabel. > > We were already addressing various reports for disklabel related code in > the kernel and userland. In userland we as far as I recall just copy the > struct into an aligned promptly pointer. > > There might be more problems, but we address them as they pop up. That seems counterintuitive. There's the root cause and when/if that root cause is fixed, then this particular problem will be fixed as well. The concern obviously is that when the root problem is fixed, this change will be forgotten and the unnecessarily uglified code will be just left over as it currently is. So the change is extremely questionable from that point of view. It creates the illusion of things being "fixed" while in the longer run I'd say it harms the code. And as I said, if the sanitizer flags that place as UB and doesn't flag dozens of dlp->foo accesses elsewhere in that file, then either I don't understand what the sanitizer complains about, or it's not a very good sanitizer and we shouldn't care to shut it up in this random place that triggers it. > > As a side note - the C99 standard contains "derefer" exactly once, in > > a footnote. Since we have ended up in the darkest corners of > > legalistic exegesis, please, can we avoid using the word that is, > > technically speaking, meaningless as far as this discussion is > > concerned? > > Unary * oprator. C++ specified term "dereferenceable" in the context of > the unary * operator. This is C code and the C standard is hard enough as it is already. Please, can we put the C++ aside for a moment? -uwe
Re: CVS commit: src/sys/kern
On Thu, Nov 07, 2019 at 06:02:39 +0100, Kamil Rytarowski wrote: > I have checked received the following patch and received a feedback from > a LLVM developer. > > On 07.11.2019 05:47, 'Dmitry Vyukov' via syzkaller-netbsd-bugs wrote: > > I've consulted with some people and _presumably_ (to the degree one > > can be sure about bitter corner cases of C/C++ :)) this is a correct > > fix (and formally correct warnings from ubsan). > > As 6.5.3.2/4 says, only &*p and [i] syntactic forms are defined as > > special case of not being a dereference, but rather effectively an > > address calculation. But >m is not. Thus it is interpreted as a > > dereference that produces an lvalue and then taking address of that > > lvalue. At the point of dereference we have UB. Your fix avoids the > > dereference. The context is lost in the thread, but the original change was about >d_magic as far as I can figure out. If the claim is that that's UB b/c dlp is improperly aligned, then why the half of the rest of the file is not UB as it uses the same "dlp" pointer to access other members of the disklabel. As a side note - the C99 standard contains "derefer" exactly once, in a footnote. Since we have ended up in the darkest corners of legalistic exegesis, please, can we avoid using the word that is, technically speaking, meaningless as far as this discussion is concerned? -uwe
Re: CVS commit: src/sys
On Wed, Oct 16, 2019 at 11:27:39 -0400, Christos Zoulas wrote: > Module Name: src > Committed By: christos > Date: Wed Oct 16 15:27:39 UTC 2019 > > Modified Files: > src/sys/kern: kern_core.c kern_hook.c kern_sig.c kern_veriexec.c > subr_ipi.c subr_pool.c subr_vmem.c sys_ptrace_common.c > src/sys/net: if_ethersubr.c > > Log Message: > Add void * function pointer casts. There are different ways to "fix" those > warnings: > 1. this one: add a void * cast (which I think is the least intrusive) > 2. add pragmas to elide the warning > 3. add intermediate inline conversion functions > 4. change the called function prototypes, adding unused arguments and >converting some of the pointer arguments to void *. > 5. make the functions varyadic (which defeats the purpose of checking) > 6. pass command line flags to elide the warning > I did try 3 and 4 and I was not pleased with the result (sys_ptrace_common.c) > (3) added too much code and defines, and (4) made the regular use clumsy. Can we maybe mark these (and others you did recently) with a macro akin to UNCONST, so that they can be found out later? FCOERCE(int (*)(void *, void *), foo) or something? -uwe
