Re: CVS commit: src/sys/arch/atari/stand/installboot

[Izumi Tsutsui]

christos@ wrote:

 Until that stops working, or being available. I think we should
 let the majority decide what's appropriate.

One major problem of the NetBSD Project is that we don't have any
well defined procedure to get majority.
(we don't have enough activities or a person like Linus unfortunately)

Persons who posted patches before commit were often blocked by bikeshed,
and persons who committed changes without review (or even tests)
always won by objecting (even just I don't think so) all post claims.
Only core might handle technical issue, but it looks core's decisions
always ignore non-technical matters (resources, marketing, users etc).

 | It's much worse to commit untested broken code.
 
 That was fixed and you tested it I presume.

Unfortunately I didn't test your code at all because I objected it
(the first rev was obviously broken anyway) and it was reverted
per your approval:
 http://mail-index.netbsd.org/source-changes-d/2014/11/15/msg007338.html

After you read other developer's post you started to claim
to strictly follow -fstrict-aliasing and use memcpy() to achieve it but
you always ignored my request (to use u_int16_t assignments consistently),
so I still cannot see what is ok for you other than memcpy (or be16enc)
since I'm not a good C programmer and it's unclear if you are ok
to use pointer conversions via (void *).

At first you answered it didn't work
 http://mail-index.netbsd.org/source-changes-d/2014/11/13/msg007326.html
and in the next you said it was legal.
 http://mail-index.netbsd.org/source-changes-d/2014/11/16/msg007356.html

Anyway, per your second mail it looks you are ok to pass a structure
pointer to a function via (void *) arg so I'll add a new function to
set u_int16_t cksum at proper offset in the boot sector using
u_int16_t assignment as current abcksum() function does.

If you still don't like it, please propose a patch which satisfies
both requests as martin said, not repeating your memcpy is ok and
-fstrict-aliasing is more important claim.
---
Izumi Tsutsui

Re: CVS commit: src/sys/arch/atari/stand/installboot

[Taylor R Campbell]

How about we take one of tsutsui@'s earlier suggestions and compile
the code in question with -fno-strict-aliasing, and revert to the
original code, until someone can go over the whole thing to clean up
the strict-aliasing violations?  That way, the bad code will work as
originally intended and will be marked (by -fno-strict-aliasing and a
big XXX in the relevant makefile) for future developers, at least.

As is, there are obviously violations, but we have papered over them
enough that GCC isn't smart enough to warn about them: the void * cast
through abcksum and the union of pointers (as opposed to pointer to
union) for bbsec in mkbootblock.  So the violations will lurk there
until someone is bitten by them and wastes a week tracking them down
like happened with the circleq macros.

Re: CVS commit: src/sys/arch/atari/stand/installboot

[Christos Zoulas]

On Nov 24, 12:06am, tsut...@ceres.dti.ne.jp (Izumi Tsutsui) wrote:
-- Subject: Re: CVS commit: src/sys/arch/atari/stand/installboot

| One major problem of the NetBSD Project is that we don't have any
| well defined procedure to get majority.
| (we don't have enough activities or a person like Linus unfortunately)

Yes, and voting about everything can quickly get expensive or abused.

| Persons who posted patches before commit were often blocked by bikeshed,
| and persons who committed changes without review (or even tests)
| always won by objecting (even just I don't think so) all post claims.
| Only core might handle technical issue, but it looks core's decisions
| always ignore non-technical matters (resources, marketing, users etc).

One likes to think that people working towards a common goal will usually
make progress in the right direction.

| Unfortunately I didn't test your code at all because I objected it
| (the first rev was obviously broken anyway) and it was reverted
| per your approval:
|  http://mail-index.netbsd.org/source-changes-d/2014/11/15/msg007338.html

I could not have tested; I asked you to test.

| After you read other developer's post you started to claim
| to strictly follow -fstrict-aliasing and use memcpy() to achieve it but
| you always ignored my request (to use u_int16_t assignments consistently),
| so I still cannot see what is ok for you other than memcpy (or be16enc)
| since I'm not a good C programmer and it's unclear if you are ok
| to use pointer conversions via (void *).

I did not ignore you, I also said that your union solution is cleaner,
but again it was pointed out that it would not always work...

| At first you answered it didn't work
|  http://mail-index.netbsd.org/source-changes-d/2014/11/13/msg007326.html
| and in the next you said it was legal.
|  http://mail-index.netbsd.org/source-changes-d/2014/11/16/msg007356.html

Obviously I cannot be right on both counts :-) But at least I am batting
50%.

| Anyway, per your second mail it looks you are ok to pass a structure
| pointer to a function via (void *) arg so I'll add a new function to
| set u_int16_t cksum at proper offset in the boot sector using
| u_int16_t assignment as current abcksum() function does.

I fine with it as along as it works and the compiler does not complain.

| If you still don't like it, please propose a patch which satisfies
| both requests as martin said, not repeating your memcpy is ok and
| -fstrict-aliasing is more important claim.

I did not propose that one but the be16enc() solution someone else
presented seems more natural (and will work on LE machines).
 
christos

Re: CVS commit: src/sys/arch/atari/stand/installboot

[Izumi Tsutsui]

christos@ wrote:

 |  http://mail-index.netbsd.org/source-changes-d/2014/11/15/msg007338.html
 
 I could not have tested; I asked you to test.

I didn't tested it either.
I noticed your obvious wrong pointer calculation by code inspection,
and it means memcpy() would confuse future maintainers which address
should be used for the cksum.

 | If you still don't like it, please propose a patch which satisfies
 | both requests as martin said, not repeating your memcpy is ok and
 | -fstrict-aliasing is more important claim.
 
 I did not propose that one but the be16enc() solution someone else
 presented seems more natural (and will work on LE machines).

You still don't understand my point:
to use u_int16_t assignment consistently.
be16enc() is a bit better them memcpy(), but not ok for me.

memcpy() and be16enc() are functions to access stream buffers.
The cksum should be an element of u_int16_t array, not part of stream
in this case.

---
Izumi Tsutsui

Re: CVS commit: src/sys/arch/atari/stand/installboot

[Christos Zoulas]

On Nov 24,  2:28am, tsut...@ceres.dti.ne.jp (Izumi Tsutsui) wrote:
-- Subject: Re: CVS commit: src/sys/arch/atari/stand/installboot

| christos@ wrote:
| 
|  |  http://mail-index.netbsd.org/source-changes-d/2014/11/15/msg007338.html
|  
|  I could not have tested; I asked you to test.
| 
| I didn't tested it either.
| I noticed your obvious wrong pointer calculation by code inspection,
| and it means memcpy() would confuse future maintainers which address
| should be used for the cksum.
| 
|  | If you still don't like it, please propose a patch which satisfies
|  | both requests as martin said, not repeating your memcpy is ok and
|  | -fstrict-aliasing is more important claim.
|  
|  I did not propose that one but the be16enc() solution someone else
|  presented seems more natural (and will work on LE machines).
| 
| You still don't understand my point:
| to use u_int16_t assignment consistently.
| be16enc() is a bit better them memcpy(), but not ok for me.
| 
| memcpy() and be16enc() are functions to access stream buffers.
| The cksum should be an element of u_int16_t array, not part of stream
| in this case.

How about this then,

christos

Index: installboot.c
===
RCS file: /cvsroot/src/sys/arch/atari/stand/installboot/installboot.c,v
retrieving revision 1.28
diff -u -u -r1.28 installboot.c
--- installboot.c   31 Mar 2014 06:32:31 -  1.28
+++ installboot.c   23 Nov 2014 18:16:05 -
@@ -55,7 +55,7 @@
 
 static voidusage(void);
 static voidoscheck(void);
-static u_int   abcksum(void *);
+static voidabcksum(void *);
 static voidsetNVpref(void);
 static voidsetIDEpar(u_int8_t *, size_t);
 static voidmkahdiboot(struct ahdi_root *, char *,
@@ -455,8 +455,7 @@
 
 gotit: /* copy code from prototype and set new checksum */
memcpy(newroot-ar_fill, tmproot.ar_fill, sizeof(tmproot.ar_fill));
-   newroot-ar_checksum = 0;
-   newroot-ar_checksum = 0x1234 - abcksum(newroot);
+   abcksum(newroot);
 
if (verbose)
printf(AHDI  boot loader: %s\n, xxb00t);
@@ -498,8 +497,7 @@
setIDEpar(bb-bb_xxboot, sizeof(bb-bb_xxboot));
 
/* set AHDI checksum */
-   *((u_int16_t *)bb-bb_xxboot + 255) = 0;
-   *((u_int16_t *)bb-bb_xxboot + 255) = 0x1234 - abcksum(bb-bb_xxboot);
+   abcksum(bb-bb_xxboot);
 
if (verbose) {
printf(Primary   boot loader: %s\n, xxb);
@@ -571,14 +569,14 @@
}
 }
 
-static u_int
-abcksum (void *bs)
+static void
+abcksum(void *bs)
 {
u_int16_t sum  = 0,
  *st  = (u_int16_t *)bs,
- *end = (u_int16_t *)bs + 256;
+ *end = (u_int16_t *)bs + 255;
 
while (st  end)
sum += *st++;
-   return(sum);
+   *st++ = 0x1234 - sum;
 }

Re: CVS commit: src/sys/arch/atari/stand/installboot

[Izumi Tsutsui]

riastradh@ wrote:

 As is, there are obviously violations, but we have papered over them
 enough that GCC isn't smart enough to warn about them: the void * cast
 through abcksum

I'd like to hear your answer of my dumb question:
http://mail-index.netbsd.org/source-changes-d/2014/11/16/msg007354.html
 Then why don't you guys also complain to fix existing abcksum() function
 which is called at the suggested memcpy?

If you think the existing abcksum() is vaild, does this one
(adding a function which does the same as abcksum) satisfy you?
Or is it still better to revert all changes and put -fno-strict-aliasing?

--- installboot.c.orig  2014-11-16 22:38:39.0 +0900
+++ installboot.c   2014-11-22 23:40:36.0 +0900
@@ -56,6 +56,7 @@
 static voidusage(void);
 static voidoscheck(void);
 static u_int   abcksum(void *);
+static voidsetabcksum(void *, u_int16_t);
 static voidsetNVpref(void);
 static voidsetIDEpar(u_int8_t *, size_t);
 static voidmkahdiboot(struct ahdi_root *, char *,
@@ -467,6 +468,7 @@
 struct disklabel *label, u_int magic)
 {
int  fd;
+   u_int16_t cksum;
 
memset(bb, 0, sizeof(*bb));
 
@@ -498,8 +500,9 @@
setIDEpar(bb-bb_xxboot, sizeof(bb-bb_xxboot));
 
/* set AHDI checksum */
-   *((u_int16_t *)bb-bb_xxboot + 255) = 0;
-   *((u_int16_t *)bb-bb_xxboot + 255) = 0x1234 - abcksum(bb-bb_xxboot);
+   setabcksum(bb-bb_xxboot, 0);
+   cksum = 0x1234 - abcksum(bb-bb_xxboot);
+   setabcksum(bb-bb_xxboot, cksum);
 
if (verbose) {
printf(Primary   boot loader: %s\n, xxb);
@@ -582,3 +585,11 @@
sum += *st++;
return(sum);
 }
+
+static void
+setabcksum(void *bs, u_int16_t sum)
+{
+   u_int16_t *cksum = (u_int16_t *)bs + 255;
+
+   *cksum = sum;
+}

---
Izumi Tsutsui

Re: CVS commit: src/sys/arch/atari/stand/installboot

[Izumi Tsutsui]

christos@ wrote:

 How about this then,
 :
 -static u_int abcksum(void *);
 +static void  abcksum(void *);

Changing existing functions requires more tests and
it's annoying for maintainers, especially for netbsd-7.

Please don't assume we have unlimited resources and
remember that we need just temporary workaround until
we get atari support in MI installboot.

 - newroot-ar_checksum = 0;
 - newroot-ar_checksum = 0x1234 - abcksum(newroot);
 + abcksum(newroot);

Did you check the definition of the struct ahdi_root?
ar_checksum is properly defined as u_int16_t and
no change is required in the mkahdiboot() function.

Actually I don't know about atari specific AHDI label at all
so it's also annyoing for me to test it.

 -static u_int
 -abcksum (void *bs)
 +static void
 +abcksum(void *bs)
  {
   u_int16_t sum  = 0,
 *st  = (u_int16_t *)bs,
 -   *end = (u_int16_t *)bs + 256;
 +   *end = (u_int16_t *)bs + 255;
  
   while (st  end)
   sum += *st++;
 - return(sum);
 + *st++ = 0x1234 - sum;

You should also consider about current MI dkcksum() (and dkcksum_sized())
implementation in sys/kern/subr_disk.c.  The MI dkcksum() just does
calculate a sum of the label and magic numbers are handled by callers:
  label-d_checksum = 0;
  label-d_checksum = dkcksum(label);

It looks better to use consistent strategies for both MI/MD sums.

---
Izumi Tsutsui

Re: CVS commit: src/sys/arch/atari/stand/installboot

[Taylor R Campbell]

   Date: Mon, 24 Nov 2014 03:20:27 +0900
   From: Izumi Tsutsui tsut...@ceres.dti.ne.jp

   riastradh@ wrote:

As is, there are obviously violations, but we have papered over them
enough that GCC isn't smart enough to warn about them: the void * cast
through abcksum

   I'd like to hear your answer of my dumb question:
   http://mail-index.netbsd.org/source-changes-d/2014/11/16/msg007354.html

`Then why don't you guys also complain to fix existing abcksum()
function which is called at the suggested memcpy?'

I didn't see it before.  The existing abcksum violates strict-aliasing
too.  That's why I suggested taking your suggestion of falling back to
-fno-strict-aliasing until someone is willing to turn it back on and
go through all the code to make it safe, or rototill the whole thing
into MI installboot.

Re: CVS commit: src/sys/arch/atari/stand/installboot

[Christos Zoulas]

On Nov 24,  4:01am, tsut...@ceres.dti.ne.jp (Izumi Tsutsui) wrote:
-- Subject: Re: CVS commit: src/sys/arch/atari/stand/installboot

|  -static u_int   abcksum(void *);
|  +static voidabcksum(void *);
| 
| Changing existing functions requires more tests and
| it's annoying for maintainers, especially for netbsd-7.

Yes, taking this approach to the extreme we should never switch compilers.

| Did you check the definition of the struct ahdi_root?
| ar_checksum is properly defined as u_int16_t and
| no change is required in the mkahdiboot() function.

Yes, carefully and it does the same thing so the code can be shared.

| You should also consider about current MI dkcksum() (and dkcksum_sized())
| implementation in sys/kern/subr_disk.c.  The MI dkcksum() just does
| calculate a sum of the label and magic numbers are handled by callers:
| label-d_checksum = 0;
| label-d_checksum = dkcksum(label);
| 
| It looks better to use consistent strategies for both MI/MD sums.

I think that the only solution you'll like is yours, so please do it
your way.

christos

Re: CVS commit: src/sys/arch/atari/stand/installboot

[Izumi Tsutsui]

I'd like to hear your answer of my dumb question:
http://mail-index.netbsd.org/source-changes-d/2014/11/16/msg007354.html
 
 `Then why don't you guys also complain to fix existing abcksum()
 function which is called at the suggested memcpy?'
 
 I didn't see it before.  The existing abcksum violates strict-aliasing
 too.  That's why I suggested taking your suggestion of falling back to
 -fno-strict-aliasing until someone is willing to turn it back on and
 go through all the code to make it safe, or rototill the whole thing
 into MI installboot.

Christos said it is legally converting a void * pointer.
 http://mail-index.netbsd.org/source-changes-d/2014/11/16/msg007356.html
You guys have different opinions.  Which is correct?

Which C99 specification you think the existing abcksum violates?

---
Izumi Tsutsui

Re: CVS commit: src/sys/arch/atari/stand/installboot

[Izumi Tsutsui]

christos@ wrote:

 |  -static u_int abcksum(void *);
 |  +static void  abcksum(void *);
 | 
 | Changing existing functions requires more tests and
 | it's annoying for maintainers, especially for netbsd-7.
 
 Yes, taking this approach to the extreme we should never switch compilers.

Please stop such extreme or nothing approach if you cannot maintain it.

 | You should also consider about current MI dkcksum() (and dkcksum_sized())
 | implementation in sys/kern/subr_disk.c.  The MI dkcksum() just does
 | calculate a sum of the label and magic numbers are handled by callers:
 |   label-d_checksum = 0;
 |   label-d_checksum = dkcksum(label);
 | 
 | It looks better to use consistent strategies for both MI/MD sums.
 
 I think that the only solution you'll like is yours, so please do it
 your way.

Could you read and answer my another post?
 http://mail-index.netbsd.org/source-changes-d/2014/11/23/msg007400.html

Taylor claims the existing abcksum() also violates aliasing rule.
 http://mail-index.netbsd.org/source-changes-d/2014/11/23/msg007402.html

If he is correct it's no sense to tweak only functions complained
by current gcc48.

---
Izumi Tsutsui

Re: CVS commit: src/sys/arch/atari/stand/installboot

[Taylor R Campbell]

   Date: Mon, 24 Nov 2014 04:46:31 +0900
   From: Izumi Tsutsui tsut...@ceres.dti.ne.jp

   Christos said it is legally converting a void * pointer.
http://mail-index.netbsd.org/source-changes-d/2014/11/16/msg007356.html
   You guys have different opinions.  Which is correct?

   Which C99 specification you think the existing abcksum violates?

Casting to void * is valid.  Converting a pointer to a uint8_t object
to uint16_t * and then dereferencing the uint16_t * is not valid, even
if you converted to uint16_t * via some void * casts.

See C99 Section 6.5 `Expressions', Clause 7, p. 68: `An object shall
have its stored value accessed only by an lvalue expression that has
one of the following types: ...'.  Casts through void * are irrelevant
here: if the object's effective type is uint8_t (as is the case for an
element of the bb_xxboot member of a struct bootblock), uint16_t is
not one of the allowed types for accessing the object.

Re: CVS commit: src/sys/arch/atari/stand/installboot

[Christos Zoulas]

On Nov 24,  4:55am, tsut...@ceres.dti.ne.jp (Izumi Tsutsui) wrote:
-- Subject: Re: CVS commit: src/sys/arch/atari/stand/installboot

|  Yes, taking this approach to the extreme we should never switch compilers.
| 
| Please stop such extreme or nothing approach if you cannot maintain it.

Ok, it is because I feel that all this discussion has gotten out of proportion.

| Could you read and answer my another post?
|  http://mail-index.netbsd.org/source-changes-d/2014/11/23/msg007400.html
| 
| Taylor claims the existing abcksum() also violates aliasing rule.
|  http://mail-index.netbsd.org/source-changes-d/2014/11/23/msg007402.html
| 
| If he is correct it's no sense to tweak only functions complained
| by current gcc48.

Yes, both solutions (your post and the existing checksum function
(with our without my changes)) violate strict aliasing rules since
they go from uint8_t * - void * - uint16_t *. Currently this
works with gcc and there are no warnings, but it is not safe.  The
sanctioned solutions are to use unions of the memory block types
or memcpy().

christos

Re: CVS commit: src/sys/arch/atari/stand/installboot

[Izumi Tsutsui]

christos@ wrote:

 | Taylor claims the existing abcksum() also violates aliasing rule.
 |  http://mail-index.netbsd.org/source-changes-d/2014/11/23/msg007402.html
 | 
 | If he is correct it's no sense to tweak only functions complained
 | by current gcc48.
 
 Yes, both solutions (your post and the existing checksum function
 (with our without my changes)) violate strict aliasing rules since
 they go from uint8_t * - void * - uint16_t *. Currently this
 works with gcc and there are no warnings, but it is not safe.  The
 sanctioned solutions are to use unions of the memory block types
 or memcpy().

Then you don't have any patch for existing (and not warned) abcksum().
Are you also okay to specify -fno-strict-aliasing with the original code
as I and mrg (and now Taylor) suggest, rather than patching only
warned assignments?

---
Izumi Tsutsui

Re: CVS commit: src/sys/arch/atari/stand/installboot

[Christos Zoulas]

On Nov 24,  7:18am, tsut...@ceres.dti.ne.jp (Izumi Tsutsui) wrote:
-- Subject: Re: CVS commit: src/sys/arch/atari/stand/installboot

| Then you don't have any patch for existing (and not warned) abcksum().
| Are you also okay to specify -fno-strict-aliasing with the original code
| as I and mrg (and now Taylor) suggest, rather than patching only
| warned assignments?

The patch works. It is not future-proof, but it works. There is
currently no way (that I know of) to flag all the strict-aliasing
violations, so the best we can do is to fix the ones the compiler
and warns about. If the compiler does not warn about them, we should
no be concerned yet (unless we find that the compiler produces
undesirable code -- like it did for CIRCLEQ and we saw how difficult
that was to fix). I still maintain that it is better to fix the
strict aliasing violations rather than papering over it with 
-fno-strict-aliasing, but I don't want to spend any more time
arguing about it.


christos

Re: CVS commit: src/sys/arch/atari/stand/installboot

[Izumi Tsutsui]

christos@ wrote:

 On Nov 24,  7:18am, tsut...@ceres.dti.ne.jp (Izumi Tsutsui) wrote:
 | Then you don't have any patch for existing (and not warned) abcksum().
 | Are you also okay to specify -fno-strict-aliasing with the original code
 | as I and mrg (and now Taylor) suggest, rather than patching only
 | warned assignments?
 
 The patch works. It is not future-proof, but it works. There is
 currently no way (that I know of) to flag all the strict-aliasing
 violations, so the best we can do is to fix the ones the compiler
 and warns about.

All suggested patches (which appease warnings) will work.
You are interested in fixing aliasing violation (while you
also suggested incorrect fix which just appeases compiler).
I would rather like to keep readability and maintainability.

To satisfy both demands, the only option is to reorganize
boot sector structures with union, but it won't happen soon
due to lack of resources and motivations.  That's all.

Anyway, -fno-strict-aliasing seems to get majority, so I'll take this one.
(there are so many other tasks blocked this dumb trouble on atari)

---
Izumi Tsutsui