Re: CVS commit: src/lib/libcurses

2022-01-16 Thread Valery Ushakov
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

2022-01-06 Thread Valery Ushakov
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

2021-12-28 Thread Valery Ushakov
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

2021-12-28 Thread Valery Ushakov
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

2021-12-07 Thread Valery Ushakov
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

2021-12-07 Thread Valery Ushakov
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

2021-11-10 Thread Valery Ushakov
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

2021-11-10 Thread Valery Ushakov
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

2021-07-26 Thread Valery Ushakov
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

2021-07-26 Thread Valery Ushakov
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

2021-04-05 Thread Valery Ushakov
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

2021-03-26 Thread Valery Ushakov
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

2021-03-26 Thread Valery Ushakov
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

2021-03-11 Thread Valery Ushakov
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)

2021-02-17 Thread Valery Ushakov
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)

2021-02-16 Thread Valery Ushakov
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)

2021-02-16 Thread Valery Ushakov
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)

2021-02-16 Thread Valery Ushakov
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)

2021-02-16 Thread Valery Ushakov
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)

2021-02-16 Thread Valery Ushakov
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

2021-02-13 Thread Valery Ushakov
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

2021-02-13 Thread Valery Ushakov
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

2021-01-26 Thread Valery Ushakov
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

2021-01-25 Thread Valery Ushakov
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

2021-01-25 Thread Valery Ushakov
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

2021-01-15 Thread Valery Ushakov
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)

2020-11-08 Thread Valery Ushakov
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

2020-08-30 Thread Valery Ushakov
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

2020-08-24 Thread Valery Ushakov
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

2020-08-24 Thread Valery Ushakov
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

2020-08-15 Thread Valery Ushakov
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

2020-08-10 Thread Valery Ushakov
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

2020-08-03 Thread Valery Ushakov
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

2020-08-03 Thread Valery Ushakov
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

2020-06-28 Thread Valery Ushakov
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

2020-06-06 Thread Valery Ushakov
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

2020-06-02 Thread Valery Ushakov
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

2020-03-14 Thread Valery Ushakov
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

2020-03-14 Thread Valery Ushakov
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

2020-03-13 Thread Valery Ushakov
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

2020-03-13 Thread Valery Ushakov
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

2020-03-13 Thread Valery Ushakov
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

2020-03-13 Thread Valery Ushakov
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

2020-03-11 Thread Valery Ushakov
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

2020-03-07 Thread Valery Ushakov
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

2020-02-24 Thread Valery Ushakov
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

2020-02-23 Thread Valery Ushakov
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

2020-02-22 Thread Valery Ushakov
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

2020-02-22 Thread Valery Ushakov
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

2020-02-22 Thread Valery Ushakov
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

2020-02-21 Thread Valery Ushakov
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

2020-02-20 Thread Valery Ushakov
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

2020-02-20 Thread Valery Ushakov
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

2020-02-13 Thread Valery Ushakov
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

2020-02-13 Thread Valery Ushakov
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

2020-02-13 Thread Valery Ushakov
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

2020-02-07 Thread Valery Ushakov
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)

2019-12-26 Thread Valery Ushakov
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

2019-12-24 Thread Valery Ushakov
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

2019-12-24 Thread Valery Ushakov
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

2019-12-23 Thread Valery Ushakov
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

2019-12-07 Thread Valery Ushakov
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

2019-12-04 Thread Valery Ushakov
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

2019-12-02 Thread Valery Ushakov
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

2019-11-07 Thread Valery Ushakov
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

2019-11-07 Thread Valery Ushakov
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

2019-11-07 Thread Valery Ushakov
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

2019-11-07 Thread Valery Ushakov
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

2019-11-07 Thread Valery Ushakov
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

2019-11-07 Thread Valery Ushakov
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

2019-11-07 Thread Valery Ushakov
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

2019-10-16 Thread Valery Ushakov
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


Re: CVS commit: src/lib/libc/db/db

2019-10-05 Thread Valery Ushakov
On Sat, Oct 05, 2019 at 18:07:58 +, Valeriy E. Ushakov wrote:

> Module Name:  src
> Committed By: uwe
> Date: Sat Oct  5 18:07:58 UTC 2019
> 
> Modified Files:
>   src/lib/libc/db/db: db.c
> 
> Log Message:
> __dberr: tweak signature to make gcc8 -Wbad-function-cast happy about
> casts in __dbpanic.  Admittedly this is a bit too "cute".
> 
> 
> To generate a diff of this commit:
> cvs rdiff -u -r1.19 -r1.20 src/lib/libc/db/db/db.c

Oops, I meant -Wcast-function-type

-uwe


Re: CVS commit: src/sys/arch/acorn32/acorn32

2019-10-04 Thread Valery Ushakov
On Fri, Oct 04, 2019 at 04:57:51 +0200, Kamil Rytarowski wrote:

> At least in certain domains of engineering 0 is a special case as it
> does not need unit (km, lumen, kg, ...) neither base (hex, dec, ..).
> 
> It is natural (correct, expected) to print %#x for 0 as 0, without 0x.

I'm happy for them and that there's %#x that they can use.  What does
it have to do with with anything?


> On 04.10.2019 04:09, Christos Zoulas wrote:
> > Thanks, and zero is special for 0#. Should I revert it?
> > 
> > christos
> > 
> >> On Oct 3, 2019, at 10:06 PM, Valery Ushakov  wrote:
> >>
> >>
> >>> Modified Files:
> >>>   src/sys/arch/acorn32/acorn32: rpc_machdep.c
> >>>
> >>> Log Message:
> >>> change 0x% -> %x
> >>
> >> This should read %#x.
> >> And this is wrong.
> >>
> >> 1) With # the 0x is part of the width, so 
> >>
> >> 0x%08x -> 0x0001
> >> %#08x  -> 0x01
> >>
> >> 0x 0x0001
> >>  0x01
> >>
> >> 2) # doesn't add prefix for zero, so 
> >>
> >> 0x%08x -> 0x
> >> %#08x  ->  
> >>
> >> -uwe
> > 
> 
> 




-uwe


Re: CVS commit: src/sys/arch/acorn32/acorn32

2019-10-03 Thread Valery Ushakov
On Thu, Oct 03, 2019 at 22:09:28 -0400, Christos Zoulas wrote:

> Thanks, and zero is special for 0#. Should I revert it?

Yes, please.


> > On Oct 3, 2019, at 10:06 PM, Valery Ushakov  wrote:
> > 
> > 
> >> Modified Files:
> >>src/sys/arch/acorn32/acorn32: rpc_machdep.c
> >> 
> >> Log Message:
> >> change 0x% -> %x
> > 
> > This should read %#x.
> > And this is wrong.
> > 
> > 1) With # the 0x is part of the width, so 
> > 
> > 0x%08x -> 0x0001
> > %#08x  -> 0x01
> > 
> > 0x 0x0001
> >  0x01
> > 
> > 2) # doesn't add prefix for zero, so 
> > 
> > 0x%08x -> 0x
> > %#08x  ->  

-uwe


Re: CVS commit: src/sys/arch/acorn32/acorn32

2019-10-03 Thread Valery Ushakov


> Modified Files:
>   src/sys/arch/acorn32/acorn32: rpc_machdep.c
> 
> Log Message:
> change 0x% -> %x

This should read %#x.
And this is wrong.

1) With # the 0x is part of the width, so 

0x%08x -> 0x0001
%#08x  -> 0x01

0x 0x0001
 0x01

2) # doesn't add prefix for zero, so 

0x%08x -> 0x
%#08x  ->  

-uwe


Re: CVS commit: src

2019-09-02 Thread Valery Ushakov
On Mon, Sep 02, 2019 at 09:08:29 +, Roy Marples wrote:

> curses(3): add curses_version()
> 
> Returns NetBSD-Curses %s
> Where %s is the NetBSD version taken from sys/param.h
> 
> Discussed on tech-net@, only for ncurses compat.

[This was on tech-userlevel@]

I know I'm late, but the patch on tech-userlevel was kinda misleading
as it used CURSES_VERSION in version.c and I didn't realize it was
actually the netbsd version from osrelease.sh. Committed code calls it
NETBSD_VERSION, which made me pay attention.

Why would we ever want to report this completely random and unrelated
fact?!

There were years when curses in the tree was unchanged.  In the mean
time we have churned through dozens of netbsd versions.

-uwe


Re: CVS commit: src/sys/dev/rasops

2019-07-28 Thread Valery Ushakov
On Sun, Jul 28, 2019 at 02:18:14 -0400, Michael wrote:

> On Sat, 27 Jul 2019 21:35:04 +0200
> Joerg Sonnenberger  wrote:
> 
> > On Fri, Jul 26, 2019 at 07:54:15AM -0300, Jared McNeill wrote:
> > > On Fri, 26 Jul 2019, Rin Okuyama wrote:
> > >   
> > > > Also, convert loop of byte-wise copy into memset.  
> > > 
> > > I am a bit concerned about this change because IIRC there are platforms
> > > where memset etc. cannot be used on device memory.  
> > 
> > memset can access memory twice or alignment depending on the platform.
> > The question is whether we care about that for the frame buffers though.
> 
> One hazard I can think of is if there's endian-twiddling involved. Some
> graphics chip / host bridge combinations will only convert accesses up
> to 32bit properly, resulting in 64bit accesses having their upper/lower
> 32bit parts swapped on delivery. That should only bite with memcpy()
> though.

I remember running into something like this with igsfb(4) on krups.

-uwe


Re: CVS commit: src/libexec/getty

2019-07-11 Thread Valery Ushakov
On Thu, Jul 11, 2019 at 09:48:23 +, Thomas Klausner wrote:

> Module Name:  src
> Committed By: wiz
> Date: Thu Jul 11 09:48:22 UTC 2019
> 
> Modified Files:
>   src/libexec/getty: gettytab.5
> 
> Log Message:
> Merge lines. Fixes display problem noted in PR 54361 by he@

This works with roff.  This used to work in older versions of mandoc
in e.g. netbsd 7.

-uwe


Re: CVS commit: src

2019-06-07 Thread Valery Ushakov
On Sat, Jun 08, 2019 at 08:20:49 +1000, matthew green wrote:

> > amd64 is probably one of the few ports that has different -p and -m
> > values (there are others, but you wouldn't normally run build.sh on
> > them :)
> 
> evbarm on modern hardware is one such platform.  uname -p can
> be a really large number of things.. and used to default to
> earmv5 even on 64 bit platforms.

Rigth, but when was the last time you build.sh on it? :)

(I did build.sh on hpcsh once just to prove the point, found a bug in
our lint too :)

-uwe


Re: CVS commit: src

2019-06-07 Thread Valery Ushakov
On Sat, Jun 08, 2019 at 01:18:03 +0900, Izumi Tsutsui wrote:

> > Fix long-term broken pattern match when determining if uname -p output is
> > valid. [^a-z] syntax isn't valid.
> 
> Does this fix PR toolchain/54100?
> http://gnats.netbsd.org/54100

No, that's unrelated.  Make uses regexps, wher ^ is correct.

The problem with the PR is the typo in the variable name, missing
leading underscore in

_HOST_ARCH:=   ${HOST_ARCH:...}

note "HOST_ARCH" in the rhs, not "_HOST_ARCH".

amd64 is probably one of the few ports that has different -p and -m
values (there are others, but you wouldn't normally run build.sh on
them :)

-uwe


Re: audio2

2019-05-28 Thread Valery Ushakov
On Tue, May 28, 2019 at 20:25:33 +0300, Andreas Gustafsson wrote:

> On May 23, Valery Ushakov wrote:
> > This feels inverted.  The mathematically correct operation required
> > here is symmetric division (that rounds towards zero). 
> 
> Rounding towards zero is absolutely the wrong thing to do for audio,
> because it introduces a discontinuity, which in turn causes
> unnecessary distortion of the audio.
> 
> For example, if your original signal is a linear ramp
> 
>   ... -5, -4, -3, -2, -1, 0, 1, 2, 3, 4, 5 ...
> 
> and you divide it by two rounding towards zero, you get
> 
>   ... -2, -2, -1, -1, 0, 0, 0, 1, 1, 2, 2 ...
> 
> where every value is repeated twice, except that the value zero occurs
> three times, so the ramp is no longer linear - it now has a visible
> and audible kink in the middle.  And this kink is in the worst
> possible place of the amplitude scale, at zero where it will affect
> even the weakest signals, distorting every zero crossing.
> 
> The proper way to do audio scaling is using dither, but failing that,
> at least the values should be consistently rounded either up or down,
> which will at worst introduce a small DC offset which you can't hear,
> but no crossover distortion.

Thanks a lot for the explanation!

Now, you omit the most critical bit: what would you name the thing? :)
>From "the proper way to do audio scaling" I infer that AUDIO_SCALEDOWN
(or however that was spelled) should be ok, shouldn't it?

-uwe


Re: audio2

2019-05-25 Thread Valery Ushakov
On Fri, May 24, 2019 at 21:51:35 +0900, Tetsuya Isaki wrote:

> At Thu, 23 May 2019 17:08:42 +0300,
> Valery Ushakov wrote:
> > > +#if defined(__GNUC__)
> > > +/* gcc defines '>>' as ASR. */
> > > +#define AUDIO_ASR(value, shift)  ((value) >> (shift))
> > > +#else
> > > +#define AUDIO_ASR(value, shift)  ((value) / (1 << (shift)))
> > > +#endif
> > 
> > This feels inverted.  The mathematically correct operation required
> > here is symmetric division (that rounds towards zero).  Arithmetic
> > shift is flooring division (that rounds towards minus infinity).
> 
> Both are not correct but are acceptable.
> 
> # I also used to feel that division is the correct operation.
> # But I don't think so now.
> 
> For example, let's consider to convert 16bits to 15bits.
> In case of round-to-zero,
>  32767, 32766   => 16383
>   :
>  3, 2   => 1
>  1, 0   => 0   < *1
>  -1 => 0   < *1
>  -2, -3 => -1
>   :
>  -32766, -32767 => -16383
>  -32768 => -16384  < *2
> 
> In case of round-to-minus-inf,
>  32767, 32766   => 16383
>   :
>  3, 2   => 1
>  1, 0   => 0
>  -1, -2 => -1
>   :
>  -32767, -32768 => -16384
> 
> As above, in round-to-zero, there are three inputs that output 0
> (*1) and there are only one input that output -16384 (*2).
> In contrast, in round-to-minus-inf, all outputs correspond to two
> input values.
> 
> Which is "correct"?

Speaking in the abstract: The operation that happens here is
rescaling, as far as I understand.  Yes, the two-complement ranges are
lopsided with an extra value on the negative side, but that is far
less important, I think, than commuting with negation.  If you negate
all samples, you have basically the same audio
https://manual.audacityteam.org/man/invert.html ("invert" is a bit
unfortuante as a name b/c of the confusion with the bitwise
operation), so it's nice to have:

-scaleN(sample) = scaleN(-sample)


> The correct operation is not exist whenever rounding to integer
> occurs.
>
> And in audio area, we need to understand that both rounding
> are not correct but are acceptable.

I think you are confusing correctness and precision here.


> Furthermore, we have to consider that this operation is performed
> in the bottom of loop in realtime mixing.  If both rounding are
> acceptable, I'd like to use faster one.  Of course, unless it is
> "undefined".
>  
> > So it feels confusing and wrong to *name* the operation the
> > opposite of what it really is.
> 
> What I want to do here is arithmetic right shift operation and 2nd
> argument in this macro indicates shift count.  So I named it ASR.

My point is exactly that you are confusing implementation detail that
uses right shift as a good enough implementation (which, I reiterate,
I don't object to here) and what is the meaning of the operation that
you are implementing/approximating (see my first paragraph above).


-uwe


Re: audio2

2019-05-23 Thread Valery Ushakov
On Thu, May 23, 2019 at 21:28:51 +0900, Tetsuya Isaki wrote:

> +/*
> + * AUDIO_ASR() does Arithmetic Shift Right operation.
> + * This macro should be used for audio wave data only.
> + *
> + * Division by power of two is replaced with shift operation in the most
> + * compiler, but even then rounding-to-zero occurs on negative value.
> + * What we handle here is the audio wave data that human hear, so we can
> + * ignore the rounding difference.  Therefore we want to use faster
> + * arithmetic shift right operation.  But the right shift operator ('>>')
> + * for negative integer is "implementation defined" behavior in C (note
> + * that it's not "undefined" behavior).  So if implementation defines '>>'
> + * as ASR, we use it.
> + *
> + * Using ASR is 1.9 times faster than division on my amd64, and 1.3 times
> + * faster on my m68k.  -- isaki 201801.
> + */
> +#if defined(__GNUC__)
> +/* gcc defines '>>' as ASR. */
> +#define AUDIO_ASR(value, shift)  ((value) >> (shift))
> +#else
> +#define AUDIO_ASR(value, shift)  ((value) / (1 << (shift)))
> +#endif

This feels inverted.  The mathematically correct operation required
here is symmetric division (that rounds towards zero).  Arithmetic
shift is flooring division (that rounds towards minus infinity).  So
it feels confusing and wrong to *name* the operation the opposite of
what it really is.

-uwe


Re: CVS commit: src/sys/arch/sh3/sh3

2019-05-09 Thread Valery Ushakov
On Thu, May 09, 2019 at 16:48:31 +, Ryo Shimizu wrote:

> Module Name:  src
> Committed By: ryo
> Date: Thu May  9 16:48:31 UTC 2019
> 
> Modified Files:
>   src/sys/arch/sh3/sh3: db_trace.c
> 
> Log Message:
> fix backtrace. it was broken.
> - use db_read_bytes() to avoid faults.
> - quite a few functions do not use frame pointers,
>   therefore always detect stack depth without a frame pointer.
>   however, since the framepointer(=r14) is used as a trapframe,
>   the code to detect the frame pointer is still needed.
> - dump the contents of trapframe during backtracing.
> - KNF

It's a bit unfortunate that this commit lumps together semantic
changes and purely cosmetic stuff (like print() instead of
(*print)()), which clutters the diff and makes the former less
obvious.

-uwe


Re: CVS commit: src/distrib/sets/lists

2019-03-18 Thread Valery Ushakov
On Tue, Mar 19, 2019 at 08:04:18 +1100, matthew green wrote:

> "Thomas Klausner" writes:
> > Module Name:src
> > Committed By:   wiz
> > Date:   Mon Mar 18 09:03:08 UTC 2019
> > 
> > Modified Files:
> > src/distrib/sets/lists/comp: ad.mips
> > src/distrib/sets/lists/debug: ad.mips
> > 
> > Log Message:
> > Remove weird lines from set lists.
> 
> was there a problem?  these lines actually mean something,
> and last time i tried this the build broke.

Right, handling these is buried in an AWK script emedded in
src/distrib/sets/sets.subr

  /^-/ {
  notwanted[substr($1, 2)] = 1;
  delete list [substr($1, 2)];
  next;
  }

-uwe


Re: CVS commit: src/sys/arch

2019-03-10 Thread Valery Ushakov
[sorry to spam the list, but your server doesn't accept mail from me
it seem]

On Sun, Mar 10, 2019 at 12:19:40 +0100, Maxime Villard wrote:
> Date: Sun, 10 Mar 2019 12:19:40 +0100
> From: Maxime Villard 
> Subject: Re: CVS commit: src/sys/arch
> To: source-changes-d@NetBSD.org, Christos Zoulas ,
>  u...@stderr.spb.ru
> 
> Le 10/03/2019 ? 11:58, Valery Ushakov a ?crit :
> > On Sun, Mar 10, 2019 at 08:30:51 +0100, Maxime Villard wrote:
> > > Date: Sun, 10 Mar 2019 08:30:51 +0100
> > > From: Maxime Villard 
> > > Subject: Re: CVS commit: src/sys/arch
> > > To: Christos Zoulas , source-changes-d@NetBSD.org
> > > 
> > > Le 10/03/2019 ? 04:25, Christos Zoulas a ?crit :
> > > > In article <20190309090956.ef19cf...@cvs.netbsd.org>,
> > > > Maxime Villard  wrote:
> > > > > -=-=-=-=-=-
> > > > > 
> > > > > Module Name:  src
> > > > > Committed By: maxv
> > > > > Date: Sat Mar  9 09:09:56 UTC 2019
> > > > > 
> > > > > Modified Files:
> > > > >   src/sys/arch/amd64/include: pmap.h
> > > > >   src/sys/arch/i386/include: pmap.h
> > > > > 
> > > > > Log Message:
> > > > > New software PTE bits.
> > > > 
> > > > Virtualbox is not booting after those two commits.
> > > 
> > > Can you elaborate? Because I've tested both amd64 and i386 on VirtualBox,
> > > and amd64 on real hardware, everything works. I've reviewed my changes
> > > from yesterday for the third time, I still don't see anything wrong.
> > 
> > Bear in mind that VirtualBox exposes a lot of host CPU features to the
> > guest.
> > 
> > -uwe
> 
> The PTE bits are standard and have the same meaning on all x86 CPUs.

That reminds me.  boot -d seems to have pmap issues, including (as
reported by other people) on real hardware.  IIRC, when -7 is stopped
in boot -d the hw pmap tree is not initialized or something.  I don't
really know x86 to interpret VBox guru properly.  More recent kerenels
don't even get to the ddb prompt, iirc.

There are more details at http://gnats.netbsd.org/53311 (modulo my
confusion about automatic bt).

Please, could you take a look when you have time?
Thanks in advance.

-uwe


Re: CVS commit: src/sys/arch

2019-03-10 Thread Valery Ushakov
On Sun, Mar 10, 2019 at 08:30:51 +0100, Maxime Villard wrote:
> Date: Sun, 10 Mar 2019 08:30:51 +0100
> From: Maxime Villard 
> Subject: Re: CVS commit: src/sys/arch
> To: Christos Zoulas , source-changes-d@NetBSD.org
> 
> Le 10/03/2019 ? 04:25, Christos Zoulas a ?crit :
> > In article <20190309090956.ef19cf...@cvs.netbsd.org>,
> > Maxime Villard  wrote:
> > > -=-=-=-=-=-
> > > 
> > > Module Name:  src
> > > Committed By: maxv
> > > Date: Sat Mar  9 09:09:56 UTC 2019
> > > 
> > > Modified Files:
> > >   src/sys/arch/amd64/include: pmap.h
> > >   src/sys/arch/i386/include: pmap.h
> > > 
> > > Log Message:
> > > New software PTE bits.
> > 
> > Virtualbox is not booting after those two commits.
> 
> Can you elaborate? Because I've tested both amd64 and i386 on VirtualBox,
> and amd64 on real hardware, everything works. I've reviewed my changes
> from yesterday for the third time, I still don't see anything wrong.

Bear in mind that VirtualBox exposes a lot of host CPU features to the
guest.

-uwe


Re: CVS commit: src/sys/netinet

2019-02-25 Thread Valery Ushakov
On Mon, Feb 25, 2019 at 06:23:33 +0100, Martin Husemann wrote:

> On Sun, Feb 24, 2019 at 09:43:52PM +0100, Kamil Rytarowski wrote:
> > I consider that this is just GCC specific behavior to make some warnings
> > fatal depending on driver configuration. This is typical behavior of GCC
> > that we keep observing all over again.
> 
> No, this is very different to optimizer and target specific warnings.
> 
> There must be a fundamental difference in includes, it should all be pretty
> obvious from comparing .depend / *.d files.
> 
> IMO this should be analyzed clearly instead of papered over.

GENERIC does not enable SCTP, but ALL does.  Do we build ALL as part
of autobuilds?

-uwe


Re: CVS commit: src/usr.bin/printf

2019-01-27 Thread Valery Ushakov
On Sun, Jan 27, 2019 at 12:03:09 +, Robert Elz wrote:

> Modified Files:
>   src/usr.bin/printf: printf.c
> 
> Log Message:
> Revert previous, it was based upon a misreading of the POSIX
> spec.   POSIX requires "as if by calling strtod()" which we
> did already ... by calling strtod().   Go back to doing that.

This was changed in Issue 7.  The link to the Austin group
interpretations repository seems to be broken, though.  Where can I
find the text for the rationale behind this?

-uwe


Re: CVS commit: src/bin/sleep

2019-01-25 Thread Valery Ushakov
On Fri, Jan 25, 2019 at 20:59:56 +0700, Robert Elz wrote:

> What other commands take floating point args on the command line
> (aside from awk assignments to vars, which are certainly intended to
> be locale specific) I'm not sure I can think of one.

I think printf(1) comes closest.

http://pubs.opengroup.org/onlinepubs/7908799/xcu/printf.html

 The argument operands [...] will be evaluated as a C constant, as
 described by the ISO C standard, with the following extensions:

- A leading plus or minus sign will be allowed.

- If the leading character is a single- or double-quote, the value
  will be the numeric value in the underlying codeset of the
  character following the single- or double-quote.

Linux:

  $ LC_NUMERIC=C /usr/bin/printf '%f\n' 3.14
  3.14
  $ LC_NUMERIC=C /usr/bin/printf '%f\n' 3,14
  /usr/bin/printf: '3,14': value not completely converted
  3.00

  $ LC_NUMERIC=ru_RU.UTF-8  /usr/bin/printf '%f\n' 3.14
  3,14
  $ LC_NUMERIC=ru_RU.UTF-8  /usr/bin/printf '%f\n' 3,14
  /usr/bin/printf: '3,141': value not completely converted
  3,00


NetBSD:

  $ LC_NUMERIC=C /usr/bin/printf '%f\n' 3.14
  3.14
  $ LC_NUMERIC=C /usr/bin/printf '%f\n' 3,14
  printf: 3,14: not completely converted
  3.00

  $ LC_NUMERIC=ru_RU.UTF-8  /usr/bin/printf '%f\n' 3.14
  printf: 3.14: not completely converted
  3,00
  $ LC_NUMERIC=ru_RU.UTF-8  /usr/bin/printf '%f\n' 3,14
  3,14


So NetBSD printf has the same problem.

-uwe


Re: CVS commit: src/bin/sleep

2019-01-25 Thread Valery Ushakov
On Fri, Jan 25, 2019 at 17:38:31 +0700, Robert Elz wrote:

> Date:Fri, 25 Jan 2019 12:30:23 +0300
> From:    Valery Ushakov 
> Message-ID:  <20190125093023.gc18...@pony.stderr.spb.ru>
> 
>   | As someone who actually have to ecnoutner locales in daily life and
>   | not just think about them sitting in an ivory tower I don't understand
>   | why do we even have this argument. Locale-specific argument to sleep
>   | is pure madness, period (no pun intended).
> 
> I am not sure why you are suddenly complaining, it has been like
> this for > 21 years...   It cannot have bothered your too much, in
> your boring old stone tower, as until this week, you don't seem to
> have complained about it.
> 
> Why now?

B/c nobody expects Spanish Inquisition?  I try to avoid locale with
the exception of LC_CTYPE.  To think that sleep(1) had such a landmine
hidden in it...  that only strengthens my resolve to avoid most locale
related things.  I don't understand why the locale support in that
particular place is not ripped out immediately when discovered.  If
the problem described in the original report is not a gross and
cynical violation of POLA I don't know what is.


> And as I said before, if this is to be changed, it needs to be via
> a discussion in front of a wider audience than reads source-changes-d
> and in particular messages with a subject that refers to one of the more
> boring commands that we have.

I have posted about this to the original thread on netbsd-users@ when
the issue came up, before any changes were made.  It looks like gmane
nntp gateway has lost my message.  It is in my =posted mailbox, but
not in the mail-index archives.

The other mail that made it to the list was about an openwindows
program in sunos (mail? i don't remember) that accidentally generated
PostScript with locale specific floating point numbers.  As you can
imagine PS interpreter didn't know how to interpret 0,1 0,1 rmoveto
This sleep fiasco is up there with that story.


-uwe


Re: CVS commit: src/bin/sleep

2019-01-25 Thread Valery Ushakov
On Fri, Jan 25, 2019 at 09:48:26 +, David Brownlee wrote:

> On Fri, 25 Jan 2019 at 09:30, Valery Ushakov  wrote:
> >
> > On Fri, Jan 25, 2019 at 10:43:10 +0700, Robert Elz wrote:
> >
> > > Date:Thu, 24 Jan 2019 16:18:49 +0100
> > > From:Joerg Sonnenberger 
> > > Message-ID:  <20190124151849.ga10...@britannica.bec.de>
> > >
> > >   | This is overcomplicated and fragile, IMO. Can we just go back to the 
> > > old
> > >   | code and switch the strtod to strtod_l with LC_C_LOCALE? That solves 
> > > the
> > >   | input problem.
> > >
> > > We could certainly do that, but while it is a little complicated, I do not
> > > really think it is fragile.  Using a locale specific decimal radix in a
> > > script is fragile - but the only way to avoid that is either to 
> > > effectively
> > > give up on non-C locales entirely for scripts, or drop all support for
> > > fractional numbers as args to any command.
> > >
> > > Note: the arg to "sleep" might come as ...
> > >   echo -n "How many seconds do you want to sleep? "
> > >   read secs
> > >   sleep "${secs}"
> > > (with some error checking).
> > >
> > > I have no idea why sleep() was made to parse its arg in a locale
> > > specific way, but that was done long long ago, and was (according
> > > to the comments) done quite deliberately.
> > >
> > > I think changing that decison (rather than just avoiding the problem,
> > > as the current "fix" does) needs more discussion than a few comments
> > > on the source-changes-d list.
> >
> > As someone who actually have to ecnoutner locales in daily life and
> > not just think about them sitting in an ivory tower I don't understand
> > why do we even have this argument. Locale-specific argument to sleep
> > is pure madness, period (no pun intended).
> 
> Granted, and no-one is arguing that sleep should not handle the
> standard '.' separator.
> 
> The only question is what it should do if it is ever called with a
> locale specific separator instead - error or take Postel's law and "be
> liberal in what you accept".
> 
> I think it might be nice to have a way to control behaviour like this
> in tools - maybe an environment variable for "disable various
> extensions and be more strict in what is accepted", but that is a
> separate bikeshed :)

Thank you for demonstrating my point about the "ivory tower". :)

-uwe


Re: CVS commit: src/bin/sleep

2019-01-25 Thread Valery Ushakov
On Fri, Jan 25, 2019 at 10:43:10 +0700, Robert Elz wrote:

> Date:Thu, 24 Jan 2019 16:18:49 +0100
> From:Joerg Sonnenberger 
> Message-ID:  <20190124151849.ga10...@britannica.bec.de>
> 
>   | This is overcomplicated and fragile, IMO. Can we just go back to the old
>   | code and switch the strtod to strtod_l with LC_C_LOCALE? That solves the
>   | input problem.
> 
> We could certainly do that, but while it is a little complicated, I do not
> really think it is fragile.  Using a locale specific decimal radix in a
> script is fragile - but the only way to avoid that is either to effectively
> give up on non-C locales entirely for scripts, or drop all support for
> fractional numbers as args to any command.
> 
> Note: the arg to "sleep" might come as ...
>   echo -n "How many seconds do you want to sleep? "
>   read secs
>   sleep "${secs}"
> (with some error checking).
> 
> I have no idea why sleep() was made to parse its arg in a locale
> specific way, but that was done long long ago, and was (according
> to the comments) done quite deliberately.
> 
> I think changing that decison (rather than just avoiding the problem,
> as the current "fix" does) needs more discussion than a few comments
> on the source-changes-d list.

As someone who actually have to ecnoutner locales in daily life and
not just think about them sitting in an ivory tower I don't understand
why do we even have this argument. Locale-specific argument to sleep
is pure madness, period (no pun intended).

-uwe


Re: CVS commit: src/bin/sleep

2019-01-24 Thread Valery Ushakov
On Thu, Jan 24, 2019 at 16:18:49 +0100, Joerg Sonnenberger wrote:
> Date: Thu, 24 Jan 2019 16:18:49 +0100
> From: Joerg Sonnenberger 
> Subject: Re: CVS commit: src/bin/sleep
> To: source-changes-d@NetBSD.org
> Mail-Followup-To: source-changes-d@NetBSD.org
> 
w
> On Sat, Jan 19, 2019 at 01:27:12PM +, Robert Elz wrote:
> > Module Name:src
> > Committed By:   kre
> > Date:   Sat Jan 19 13:27:12 UTC 2019
> > 
> > Modified Files:
> > src/bin/sleep: sleep.c
> > 
> > Log Message:
> > Allow the decimal radix character '.' to work, regardless of
> > what the current locale's radix character happens to be,
> > while still allowing locale specific entry of fractional
> > seconds (ie: if you're in locale where the radix character
> > is ',' you san use "sleep 2.5" or "sleep 2,5" and they
> > accomplish the same thing).
> 
> This is overcomplicated and fragile, IMO. Can we just go back to the old
> code and switch the strtod to strtod_l with LC_C_LOCALE? That solves the
> input problem.

Seconded.  Accepting locale specific number formats here is quite
against POLA, imho.

-uwe


Re: CVS commit: src/lib/libwrap

2019-01-11 Thread Valery Ushakov
On Fri, Jan 11, 2019 at 07:39:58 +0700, Robert Elz wrote:

> farily easy to add, if only gcc didn't go and do printf format string
> analysis and complain when it sees something it does not understand.

gcc has plugin API that lets you add support for custom format
languages.  We use it at work where we have effectively our own
superset of printf with lots of additional formatters.

-uwe


Re: CVS commit: src

2018-12-30 Thread Valery Ushakov
On Sun, Dec 30, 2018 at 18:19:41 -0500, Christos Zoulas wrote:

> Modified Files:
>   src: build.sh
> 
> Log Message:
> add build libs (undocumented).

Just curious, what is it for?  Also build.sh -V NOBINARIES= build
already does more or less the same, isn't it?

-uwe


Re: CVS commit: src/sys

2018-11-30 Thread Valery Ushakov
On Fri, Nov 30, 2018 at 13:59:11 +0100, Martin Husemann wrote:

> On Fri, Nov 30, 2018 at 07:47:38AM -0500, Christos Zoulas wrote:
> > We need to see if the spec says so. I am not sure if we can depend
> > that an implementation does it.
> 
> The spec only talks about members not named in the list - I can't see
> any hints about padding. 
> 
> Might be worth to ask some std gurus for clarification.

I'm not one, but search for "padding" in 6.2.6.1 General (in 6.2.6
Representations of types).

   [#6]  When  a  value  is stored in an object of structure or
   union type, including in a member object, the bytes  of  the
   object  representation  that correspond to any padding bytes
   take  unspecified  values.42)

   42)Thus,  for   example,   structure   assignment   may   be
  implemented element-at-a-time or via memcpy.

so padding may contain garbage.

-uwe


Re: CVS commit: src/sys/sys

2018-11-06 Thread Valery Ushakov
On Tue, Nov 06, 2018 at 16:26:44 +, Maya Rashish wrote:

> Module Name:  src
> Committed By: maya
> Date: Tue Nov  6 16:26:44 UTC 2018
> 
> Modified Files:
>   src/sys/sys: stdint.h types.h
> 
> Log Message:
> Guard from type redefinition (needed by pre-C11 C) in a safer way.
> 
> The existing way causes problems like:
> https://mail-index.netbsd.org/tech-pkg/2018/10/25/msg020395.html
> https://mail-index.netbsd.org/tech-userlevel/2018/09/08/msg011381.html

I would argue that the first example is, if not outright wrong, then
at the very minimum unhygienic, as it doesn't use the same
preprocessor nesting for the definition of the name and the use of the
name, i.e. it incorrectly assumes that the function will be named
"something_uint32_t".

Also your change breaks redefining intN_t types with the preprocessor.
E.g.

#define uint32_t unsigned long long
#include 

is now broken with your change.

-uwe


Re: CVS commit: src/distrib/sets/lists/comp

2018-10-27 Thread Valery Ushakov
On Sat, Oct 27, 2018 at 07:24:59 +, Kamil Rytarowski wrote:

> Module Name:  src
> Committed By: kamil
> Date: Sat Oct 27 07:24:58 UTC 2018
> 
> Modified Files:
>   src/distrib/sets/lists/comp: mi
> 
> Log Message:
> Register missing files in distrib sets
> 
> Add curses_insch.0, mvinsch.0 and mvwinsch.0.

Somehow I was sure we dropped prebuilt catman support?  If not, the
other cat pages need to be restored too.

-uwe


  1   2   >