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

[Izumi Tsutsui]

Module Name:src
Committed By:   tsutsui
Date:   Sun Jul  3 16:16:50 UTC 2022

Modified Files:
src/sys/arch/atari/stand/installboot: Makefile installboot.c

Log Message:
Fix inverted logic.  My fault back in 2015..


To generate a diff of this commit:
cvs rdiff -u -r1.10 -r1.11 src/sys/arch/atari/stand/installboot/Makefile
cvs rdiff -u -r1.38 -r1.39 src/sys/arch/atari/stand/installboot/installboot.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.

Modified files:

Index: src/sys/arch/atari/stand/installboot/Makefile
diff -u src/sys/arch/atari/stand/installboot/Makefile:1.10 src/sys/arch/atari/stand/installboot/Makefile:1.11
--- src/sys/arch/atari/stand/installboot/Makefile:1.10	Wed May 11 10:36:52 2022
+++ src/sys/arch/atari/stand/installboot/Makefile	Sun Jul  3 16:16:50 2022
@@ -1,4 +1,4 @@
-#	$NetBSD: Makefile,v 1.10 2022/05/11 10:36:52 rin Exp $
+#	$NetBSD: Makefile,v 1.11 2022/07/03 16:16:50 tsutsui Exp $
 
 WARNS=	4
 PROG=	installboot
@@ -6,7 +6,7 @@ NOMAN=	# defined
 SRCS=	installboot.c disklabel.c
 BINDIR=	/usr/mdec
 .ifndef SMALLPROG
-CPPFLAGS+=	-DCHECK_OS_BOOTVERSION -DNO_USAGE -DSUPPORT_FD
+CPPFLAGS+=	-DCHECK_OS_BOOTVERSION -DUSAGE -DSUPPORT_FD
 LDADD=	-lkvm
 .endif
 

Index: src/sys/arch/atari/stand/installboot/installboot.c
diff -u src/sys/arch/atari/stand/installboot/installboot.c:1.38 src/sys/arch/atari/stand/installboot/installboot.c:1.39
--- src/sys/arch/atari/stand/installboot/installboot.c:1.38	Wed May 11 10:31:12 2022
+++ src/sys/arch/atari/stand/installboot/installboot.c	Sun Jul  3 16:16:50 2022
@@ -1,4 +1,4 @@
-/*	$NetBSD: installboot.c,v 1.38 2022/05/11 10:31:12 rin Exp $	*/
+/*	$NetBSD: installboot.c,v 1.39 2022/07/03 16:16:50 tsutsui Exp $	*/
 
 /*
  * Copyright (c) 1995 Waldi Ravens
@@ -86,7 +86,7 @@ usage(void)
 {
 	fprintf(stderr,
 		"usage: installboot [options] device\n"
-#ifndef NO_USAGE
+#ifdef USAGE
 		"where options are:\n"
 		"\t-N  do not actually write anything on the disk\n"
 		"\t-m  use Milan boot blocks\n"

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

[Izumi Tsutsui]

Module Name:src
Committed By:   tsutsui
Date:   Sun Jul  3 16:16:50 UTC 2022

Modified Files:
src/sys/arch/atari/stand/installboot: Makefile installboot.c

Log Message:
Fix inverted logic.  My fault back in 2015..


To generate a diff of this commit:
cvs rdiff -u -r1.10 -r1.11 src/sys/arch/atari/stand/installboot/Makefile
cvs rdiff -u -r1.38 -r1.39 src/sys/arch/atari/stand/installboot/installboot.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.

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

[Rin Okuyama]

Module Name:src
Committed By:   rin
Date:   Wed May 11 10:31:12 UTC 2022

Modified Files:
src/sys/arch/atari/stand/installboot: installboot.c

Log Message:
Make FD support optional (intended for install media).


To generate a diff of this commit:
cvs rdiff -u -r1.37 -r1.38 src/sys/arch/atari/stand/installboot/installboot.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.

Modified files:

Index: src/sys/arch/atari/stand/installboot/installboot.c
diff -u src/sys/arch/atari/stand/installboot/installboot.c:1.37 src/sys/arch/atari/stand/installboot/installboot.c:1.38
--- src/sys/arch/atari/stand/installboot/installboot.c:1.37	Wed May 11 10:27:45 2022
+++ src/sys/arch/atari/stand/installboot/installboot.c	Wed May 11 10:31:12 2022
@@ -1,4 +1,4 @@
-/*	$NetBSD: installboot.c,v 1.37 2022/05/11 10:27:45 rin Exp $	*/
+/*	$NetBSD: installboot.c,v 1.38 2022/05/11 10:31:12 rin Exp $	*/
 
 /*
  * Copyright (c) 1995 Waldi Ravens
@@ -65,7 +65,9 @@ static void	mkahdiboot(struct ahdi_root 
 		char *, u_int32_t);
 static void	mkbootblock(struct bootblock *, char *,
 char *, struct disklabel *, u_int);
+#ifdef SUPPORT_FD
 static void	install_fd(char *, struct disklabel *);
+#endif
 static void	install_hd(char *, struct disklabel *, bool);
 
 static struct bootblock	bootarea;
@@ -165,9 +167,11 @@ main(int argc, char *argv[])
 		++devchr;
 
 	switch (*devchr) {
+#ifdef SUPPORT_FD
 		case 'f': /* fd */
 			install_fd(dn, );
 			break;
+#endif
 		case 'w': /* wd */
 			use_wd = true;
 			/* FALLTHROUGH */
@@ -218,6 +222,7 @@ oscheck(void)
 }
 #endif
 
+#ifdef SUPPORT_FD
 static void
 install_fd(char *devnm, struct disklabel *label)
 {
@@ -272,6 +277,7 @@ install_fd(char *devnm, struct disklabel
 			printf("Boot block installed on %s\n", devnm);
 	}
 }
+#endif /* SUPPORT_FD */
 
 static void
 install_hd(char *devnm, struct disklabel *label, bool use_wd)

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

[Rin Okuyama]

Module Name:src
Committed By:   rin
Date:   Wed May 11 10:31:12 UTC 2022

Modified Files:
src/sys/arch/atari/stand/installboot: installboot.c

Log Message:
Make FD support optional (intended for install media).


To generate a diff of this commit:
cvs rdiff -u -r1.37 -r1.38 src/sys/arch/atari/stand/installboot/installboot.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.

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

[Rin Okuyama]

Module Name:src
Committed By:   rin
Date:   Wed May 11 10:27:45 UTC 2022

Modified Files:
src/sys/arch/atari/stand/installboot: installboot.c

Log Message:
Refactor install_[sw]d() into install_hd().

Shave off ~0.5KB from install floppy, and dedup codes a lot.

Thanks tsutsui@ for kind review!


To generate a diff of this commit:
cvs rdiff -u -r1.36 -r1.37 src/sys/arch/atari/stand/installboot/installboot.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.

Modified files:

Index: src/sys/arch/atari/stand/installboot/installboot.c
diff -u src/sys/arch/atari/stand/installboot/installboot.c:1.36 src/sys/arch/atari/stand/installboot/installboot.c:1.37
--- src/sys/arch/atari/stand/installboot/installboot.c:1.36	Wed Jan 11 18:32:48 2017
+++ src/sys/arch/atari/stand/installboot/installboot.c	Wed May 11 10:27:45 2022
@@ -1,4 +1,4 @@
-/*	$NetBSD: installboot.c,v 1.36 2017/01/11 18:32:48 christos Exp $	*/
+/*	$NetBSD: installboot.c,v 1.37 2022/05/11 10:27:45 rin Exp $	*/
 
 /*
  * Copyright (c) 1995 Waldi Ravens
@@ -66,8 +66,7 @@ static void	mkahdiboot(struct ahdi_root 
 static void	mkbootblock(struct bootblock *, char *,
 char *, struct disklabel *, u_int);
 static void	install_fd(char *, struct disklabel *);
-static void	install_sd(char *, struct disklabel *);
-static void	install_wd(char *, struct disklabel *);
+static void	install_hd(char *, struct disklabel *, bool);
 
 static struct bootblock	bootarea;
 static struct ahdi_root ahdiboot;
@@ -104,6 +103,7 @@ main(int argc, char *argv[])
 	char		 *dn;
 	char		 *devchr;
 	int		 fd, c;
+	bool		 use_wd = false;
 
 #ifdef CHECK_OS_BOOTVERSION
 	/* check OS bootversion */
@@ -169,11 +169,10 @@ main(int argc, char *argv[])
 			install_fd(dn, );
 			break;
 		case 'w': /* wd */
-			install_wd(dn, );
-			setNVpref();
-			break;
+			use_wd = true;
+			/* FALLTHROUGH */
 		case 's': /* sd */
-			install_sd(dn, );
+			install_hd(dn, , use_wd);
 			setNVpref();
 			break;
 		default:
@@ -275,13 +274,19 @@ install_fd(char *devnm, struct disklabel
 }
 
 static void
-install_sd(char *devnm, struct disklabel *label)
+install_hd(char *devnm, struct disklabel *label, bool use_wd)
 {
 	const char	 *machpath;
 	char		 *xxb00t, *xxboot, *bootxx;
 	struct disklabel rawlabel;
 	u_int32_t	 bbsec;
 	u_int		 magic;
+	char		 disktype;
+
+	if (use_wd)
+		disktype = 'w';
+	else
+		disktype = 's';
 
 	if (label->d_partitions[0].p_size == 0)
 		errx(EXIT_FAILURE, "%s: No root-filesystem.", devnm);
@@ -302,7 +307,7 @@ install_sd(char *devnm, struct disklabel
 	if (bbsec) {
 		size_t xxb00tlen = strlen(mdecpath) + strlen(machpath) + 14;
 		xxb00t = alloca(xxb00tlen);
-		snprintf(xxb00t, xxb00tlen, "%s%ssdb00t.ahdi", mdecpath, machpath);
+		snprintf(xxb00t, xxb00tlen, "%s%s%cdb00t.ahdi", mdecpath, machpath, disktype);
 		xxboot = alloca(xxb00tlen);
 		snprintf(xxboot, xxb00tlen, "%s%sxxboot.ahdi", mdecpath, machpath);
 		magic = AHDIMAGIC;
@@ -310,14 +315,15 @@ install_sd(char *devnm, struct disklabel
 		size_t xxbootlen = strlen(mdecpath) + strlen(machpath) + 8;
 		xxb00t = NULL;
 		xxboot = alloca(xxbootlen);
-		snprintf(xxboot, xxbootlen, "%s%ssdboot", mdecpath, machpath);
+		snprintf(xxboot, xxbootlen, "%s%s%cdboot", mdecpath, machpath, disktype);
 		magic = NBDAMAGIC;
 	}
 	size_t bootxxlen = strlen(mdecpath) + strlen(machpath) + 8;
 	bootxx = alloca(bootxxlen);
 	snprintf(bootxx, bootxxlen, "%s%sbootxx", mdecpath, machpath);
 
-	trackpercyl = secpertrack = 0;
+	if (!use_wd)
+		trackpercyl = secpertrack = 0;
 	if (xxb00t)
 		mkahdiboot(, xxb00t, devnm, bbsec);
 	mkbootblock(, xxboot, bootxx, label, magic);
@@ -351,82 +357,6 @@ install_sd(char *devnm, struct disklabel
 }
 
 static void
-install_wd(char *devnm, struct disklabel *label)
-{
-	const char	 *machpath;
-	char		 *xxb00t, *xxboot, *bootxx;
-	struct disklabel rawlabel;
-	u_int32_t	 bbsec;
-	u_int		 magic;
-
-	if (label->d_partitions[0].p_size == 0)
-		errx(EXIT_FAILURE, "%s: No root-filesystem.", devnm);
-	if (label->d_partitions[0].p_fstype != FS_BSDFFS)
-		errx(EXIT_FAILURE, "%s: %s: Illegal root-filesystem type.",
-		 devnm, fstypenames[label->d_partitions[0].p_fstype]);
-
-	bbsec = readdisklabel(devnm, );
-	if (bbsec == NO_BOOT_BLOCK)
-		errx(EXIT_FAILURE, "%s: No NetBSD boot block.", devnm);
-	if (memcmp(label, , sizeof(*label)))
-		errx(EXIT_FAILURE, "%s: Invalid NetBSD boot block.", devnm);
-
-	if (milan)
-		machpath = milanpath;
-	else
-		machpath = stdpath;
-	if (bbsec) {
-		size_t xxb00tlen = strlen(mdecpath) + strlen(machpath) + 14;
-		xxb00t = alloca(xxb00tlen);
-		snprintf(xxb00t, xxb00tlen, "%s%swdb00t.ahdi", mdecpath, machpath);
-		xxboot = alloca(xxb00tlen);
-		snprintf(xxboot, xxb00tlen, "%s%sxxboot.ahdi", mdecpath, machpath);
-		magic = AHDIMAGIC;
-	} else {
-		size_t xxbootlen = strlen(mdecpath) + strlen(machpath) + 8;
-		xxb00t = NULL;
-		xxboot = alloca(xxbootlen);
-		snprintf(xxboot, xxbootlen, "%s%swdboot", mdecpath, 

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

[Rin Okuyama]

Module Name:src
Committed By:   rin
Date:   Wed May 11 10:27:45 UTC 2022

Modified Files:
src/sys/arch/atari/stand/installboot: installboot.c

Log Message:
Refactor install_[sw]d() into install_hd().

Shave off ~0.5KB from install floppy, and dedup codes a lot.

Thanks tsutsui@ for kind review!


To generate a diff of this commit:
cvs rdiff -u -r1.36 -r1.37 src/sys/arch/atari/stand/installboot/installboot.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.

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

[David Laight]

On Fri, Nov 14, 2014 at 02:43:39AM +0900, Izumi Tsutsui wrote:
 christos@ wrote:
 
  Module Name:src
  Committed By:   christos
  Date:   Thu Nov 13 17:19:29 UTC 2014
  
  Modified Files:
  src/sys/arch/atari/stand/installboot: installboot.c
  
  Log Message:
  fix strict aliasing violations
 
  -   *((u_int16_t *)bb-bb_xxboot + 255) = 0;
  -   *((u_int16_t *)bb-bb_xxboot + 255) = 0x1234 - abcksum(bb-bb_xxboot);
  +   sum = 0;
  +   memcpy(bb-bb_xxboot + 255, sum, sizeof(sum));
  +   sum = 0x1234 - abcksum(bb-bb_xxboot);
  +   memcpy(bb-bb_xxboot + 255, sum, sizeof(sum));
 
 I doubt your bb-bb_xxboot + 255 is the same place
 as the original (u_int16_t *)bb-bb_xxboot + 255
 (the cksum word looks at the end of 512 byte sector).
 
 memcpy(9) looks also awful for readers because it hides endianness..

(Having read all the thread...)

I'm pretty sure you can cast between a pointer to a union and
a pointer to one of its mmembers.

So under the assumption that the compiler will let you alias between
members of a union (if it doesn't then far more stuff will break)
the following is safe:

Define a union that contains a 'bb' and a u_int16_t [].
Declare a variable that is a pointer to the union.
Assign (with cast) the address of 'bb' to the pointer variable.
Access the other member of the union.

The only other alternative is to do all the accesses as 'char'.

I'd guess that if the compiler inlines memcpy() it can 'know' about the
types of the variables involved - and apply the 'strict aliasing'
rules in the same way that is applied the 'alignment' ones.

David

-- 
David Laight: da...@l8s.co.uk

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

[David Laight]

On Mon, Nov 24, 2014 at 02:36:51PM +, Taylor R Campbell wrote:
 
 In the case of abcksum, if you have
 
 uint8_t p[512];
 ...
 *((uint16_t *)p + 255) = 0;
 *((uint16_t *)p + 255) = abcksum(p);
 
 GCC may choose to evaluate abcksum before the zero assignment, because
 no uint16_t is allowed to alias a uint8_t, so the assignment of an
 lvalue with type uint16_t can't change the value of abcksum(p).

Not sure, the accesses of p[] inside abcksum() are allowed to alias
any other memory - this includes wherever was written to by the
*((uint16_t *)xxx) = 0; regardless of any expected knowledge of
the value of xxx.

For gcc you can add asm volatile(:::memory); to force it to
'forget' anything it thinks it knows about the contents of memory.

The original checksum code can be (mostly) fixed by only having
a single (uint16_t) cast.
You are then only left with problems caused if the checksum is inlined
and any earlier writes to the sector itself are still 'pending'.

David

-- 
David Laight: da...@l8s.co.uk

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

[matthew green]

 Module Name:  src
 Committed By: tsutsui
 Date: Mon Nov 24 07:52:04 UTC 2014
 
 Modified Files:
   src/sys/arch/atari/stand/installboot: Makefile
 
 Log Message:
 Specify -fno-strict-aliasing as a temporary workaround for gcc48.
 
 The existing abcksum() also violates strict-aliasing rule
 (while current gcc48 doesn't warn it) and fixing all violations
 strictly requires whole reorganization of boot sector structures.
 But it won't happen soon and this MD installboot should be integrated
 into MI installboot(8) in future, and it requires whole overhaul anyway.
 See long discussion in source-changes-d@ for details.
 
 Should be pulled up to netbsd-7 if we switches m68k to using gcc48.

i recommend pulling it up anyway.  maybe someone wants to run gcc 4.8 :-)


.mrg.

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

[matthew green]

Nick Hudson writes:
 On 11/24/14 07:52, Izumi Tsutsui wrote:
  Module Name:src
  Committed By:   tsutsui
  Date:   Mon Nov 24 07:52:04 UTC 2014
 
  Modified Files:
  src/sys/arch/atari/stand/installboot: Makefile
 
  Log Message:
  Specify -fno-strict-aliasing as a temporary workaround for gcc48.
 
  The existing abcksum() also violates strict-aliasing rule
  (while current gcc48 doesn't warn it) and fixing all violations
  strictly requires whole reorganization of boot sector structures.
  But it won't happen soon and this MD installboot should be integrated
  into MI installboot(8) in future, and it requires whole overhaul anyway.
  See long discussion in source-changes-d@ for details.
 
  Should be pulled up to netbsd-7 if we switches m68k to using gcc48.
 
 Why not pullup in anticipation?

infact, given the strict aliasing violations in this file, there
is no guarantee that GCC 4.5 isn't miscompiling it already just
without the additional warnings in 4.8...

ie, this seems sane regardless of what happens.  same for -6?


.mrg.

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

[Izumi Tsutsui]

mrg@ wrote:

 Nick Hudson writes:
   Log Message:
   Specify -fno-strict-aliasing as a temporary workaround for gcc48.
 :
   Should be pulled up to netbsd-7 if we switches m68k to using gcc48.
  
  Why not pullup in anticipation?

Because it doesn't give any visible benefits (except theoretical
correctness) for future release users, if we choose gcc45 for netbsd-7?

 infact, given the strict aliasing violations in this file, there
 is no guarantee that GCC 4.5 isn't miscompiling it already just
 without the additional warnings in 4.8...
 
 ie, this seems sane regardless of what happens.  same for -6?

I don't think it's worth because binaries have worked without changes.
(if the compiler tries reorder optimization per strict-aliasing rule
 I guess wrong implementation will be warned at that time)

IMO, all changes against release branches should have visible benefits
(fixing errors, adding new features, to make future pullups easier etc)
per possible botches.

Anyway it has less priority in my todo to test it with gcc45.
If someone[TM] can test builds etc. and send a pullup request,
it's no problem for me.

---
Izumi Tsutsui

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

[Taylor R Campbell]

   Date: Mon, 24 Nov 2014 22:39:24 +0900
   From: Izumi Tsutsui tsut...@ceres.dti.ne.jp

   I don't think it's worth because binaries have worked without changes.
   (if the compiler tries reorder optimization per strict-aliasing rule
I guess wrong implementation will be warned at that time)

No -- GCC will only warn by default if it can prove you're violating
the strict-aliasing rules.  It may not warn if it merely makes a
decision on the basis of strict-aliasing rules.

For example, in

struct foo { int x; };
struct bar { int y; };

int
frob(struct foo *f, struct bar *b)
{

f-x = 0;
b-y = 1;
return f-x;
}

GCC may (and in fact does) generate code for frob that always returns
0, because a struct foo is not allowed to alias a struct bar, so the
`b-y = 1' assignment can't change f-x.

This code does not itself violate strict-aliasing rules, but if you
call it with

struct foo f;
frob(f, (struct bar *)f);

then you have violated the rule, and you will get the `wrong' answer
(because this is undefined behaviour).  GCC may not warn about the
cast: if frob is in a different compilation unit, GCC may not know
that frob is going to dereference both pointers.

In the case of abcksum, if you have

uint8_t p[512];
...
*((uint16_t *)p + 255) = 0;
*((uint16_t *)p + 255) = abcksum(p);

GCC may choose to evaluate abcksum before the zero assignment, because
no uint16_t is allowed to alias a uint8_t, so the assignment of an
lvalue with type uint16_t can't change the value of abcksum(p).

The result would be the same as writing simply

uint8_t p[512];
...
*((uint16_t *)p + 255) = abcksum(p);

which is almost certainly not what you intend.

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

[Izumi Tsutsui]

 GCC may choose

Ok, ok.

If you can confirm current netbsd-7 or netbsd-6 binaries are already broken,
please let me know.

---
Izumi Tsutsui

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

[Taylor R Campbell]

   Date: Mon, 24 Nov 2014 23:50:12 +0900
   From: Izumi Tsutsui tsut...@ceres.dti.ne.jp

   If you can confirm current netbsd-7 or netbsd-6 binaries are already broken,
   please let me know.

I can't say without actually scrutinizing the generated code.  It
would be safer to just set -fno-strict-aliasing since the code does
rely on violating the rules.

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

[Izumi Tsutsui]

If you can confirm current netbsd-7 or netbsd-6 binaries are already 
 broken,
please let me know.
 
 I can't say without actually scrutinizing the generated code.  It
 would be safer to just set -fno-strict-aliasing since the code does
 rely on violating the rules.

Again, this is ancient MD code, and at least it worked in NetBSD 6.1.

Actual results are more important than theoretical correctness for
poor TierII users so it has low priority in my todo, as you don't
bother to scrutinize the generated code.

---
Izumi Tsutsui

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

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

[matthew green]

 | -fno-strict-aliasing in MD is acceptable compromise for me.
 
 Until that stops working, or being available. I think we should
 let the majority decide what's appropriate.

if -fstrict-aliasing becomes the only option than we'll need a
few years to clean up the kernel.

ie, i don't think this is a real problem.

what's appropriate is having something that works, and that the
maintainers of the code accept.

i tend to agree with christos about what would be the best, but
i'm OK with tsutsui using an already heavily used flag to avoid
a problem in code he maintains.  i've often been unable to work
on a port-sparc specific issue because some change was not well
tested, so instead of advancing the state of the art, i'm stuck
playing catch up all the time.  i understand where he's coming
from.


.mrg.

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

[Izumi Tsutsui]

christos@ wrote:

 | I still don't understand why people don't want one additional
 | -fno-strict-aliasing even for Tier-II ports...
 | 
 http://nxr.netbsd.org/search?q=fno-strict-aliasingproject=srcdefs=refs=path=%2Fsrc%2Fsys
 
 You evolve the code with the language, and you fix things as they come
 along. If we did not do that we would still be in the age of KR. Just
 because a situation is not perfect we don't aim to make it worse.

Your strategy is still best or nothing.
My goal is acceptable compromise because
we have fewer resources than old days.
That's all.

---
Izumi Tsutsui

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

[Christos Zoulas]

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

| christos@ wrote:
| 
|  | I still don't understand why people don't want one additional
|  | -fno-strict-aliasing even for Tier-II ports...
|  | 
http://nxr.netbsd.org/search?q=fno-strict-aliasingproject=srcdefs=refs=path=%2Fsrc%2Fsys
|  
|  You evolve the code with the language, and you fix things as they come
|  along. If we did not do that we would still be in the age of KR. Just
|  because a situation is not perfect we don't aim to make it worse.
| 
| Your strategy is still best or nothing.
| My goal is acceptable compromise because
| we have fewer resources than old days.

Best for me would have been MI installboot; compromise is make it compile
correctly without special handling.

christos

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

[Izumi Tsutsui]

christos@ wrote:

 Best for me would have been MI installboot; compromise is make it compile
 correctly without special handling.

Well, it's still your personalized opinion.
-fno-strict-aliasing in MD is acceptable compromise for me.
It's much worse to commit untested broken code.

If responsible TierII users must follow core's
(or other non-user developer's) non-essencial claims
about dumb MD source implementation, no one will maintain
such ports.

---
Izumi Tsutsui

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

[Christos Zoulas]

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

| christos@ wrote:
| 
|  Best for me would have been MI installboot; compromise is make it compile
|  correctly without special handling.
| 
| Well, it's still your personalized opinion.

Opinions are usually that way (personal).
In this case others happen to share it.

| -fno-strict-aliasing in MD is acceptable compromise for me.

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

| It's much worse to commit untested broken code.

That was fixed and you tested it I presume.

christos

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

[Izumi Tsutsui]

christos@ wrote:

 |  Yes be16enc() should work just fine, I think.
 | 
 | Then why don't you guys also complain to fix existing abcksum() function
 | which is called at the suggested memcpy?
 
 Because it is legally converting a void * pointer.

So you are a person who are just interested in correctness
but ignoring readabilty (no one can see why one uses normal
assignments and others uses stream).  Why don't you change
the cksum assignment to convert via void * pointer?

Anyway, you are claiming TierII users can't choose implementation
while our port page claims they have responsibility.
I've been really tired and lost my motivation.
Sorry.

---
Izumi Tsutsui

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

[Christos Zoulas]

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

| So you are a person who are just interested in correctness
| but ignoring readabilty (no one can see why one uses normal
| assignments and others uses stream).  Why don't you change
| the cksum assignment to convert via void * pointer?

Why does everything have to be personalized and turned into an
argument? I am not interested in correctness and ignoring
readability, I am trying to come with a mutually amicable solution.
The compiler does not like the current state of affairs.

| Anyway, you are claiming TierII users can't choose implementation
| while our port page claims they have responsibility.
| I've been really tired and lost my motivation.
| Sorry.

Again, this is not a confrontation. Let's do the following:

Since you have the hardware and it is easiest for you, can
you please make a disk image of the resulting boot block and
put it up somewhere. Then I will take the MD installboot bits
and migrate them to the MI installboot and make sure that the
MI installboot binary produces the same disk image. Then you
can test.

Does this sound reasonable?

christos

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

[Izumi Tsutsui]

christos@ wrote:

 | So you are a person who are just interested in correctness
 | but ignoring readabilty (no one can see why one uses normal
 | assignments and others uses stream).  Why don't you change
 | the cksum assignment to convert via void * pointer?
 
 Why does everything have to be personalized and turned into an
 argument? I am not interested in correctness and ignoring
 readability,

You don't think consistently using uint16_t assingments is not necessary.
I think it's necessary to explicitly describe how cksum should be
caluclated and written.  Both are personal opinions, and I don't think
there is a right answer.

 I am trying to come with a mutually amicable solution.
 The compiler does not like the current state of affairs.

As I said such best or nothing strategy is not necessary in
such MD code, especially for netbsd-7 (-fno-strict-ailasing is enough).
You always want complete solusion.  These are also personal opinions.

 | Anyway, you are claiming TierII users can't choose implementation
 | while our port page claims they have responsibility.
 | I've been really tired and lost my motivation.
 | Sorry.
 
 Again, this is not a confrontation. Let's do the following:
 
 Since you have the hardware and it is easiest for you, can
 you please make a disk image of the resulting boot block and
 put it up somewhere. Then I will take the MD installboot bits
 and migrate them to the MI installboot and make sure that the
 MI installboot binary produces the same disk image. Then you
 can test.
 
 Does this sound reasonable?

Sounds unlikely for netbsd-7.

I'm afraid you don't read how the MD installboot.c does weird operations
at all.

---
Izumi Tsutsui

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

[Martin Husemann]

On Mon, Nov 17, 2014 at 11:48:49PM +0900, Izumi Tsutsui wrote:
 You don't think consistently using uint16_t assingments is not necessary.
 I think it's necessary to explicitly describe how cksum should be
 caluclated and written.  Both are personal opinions, and I don't think
 there is a right answer.

I'm sorry, but I have a hard time following the whole argument - there has
to be a way to write the code clearly and readable but still avoid undefined
behaviour (and not paper over it by disabling some optimizations in the
current compiler).

Martin

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

[Taylor R Campbell]

   Date: Mon, 17 Nov 2014 15:55:56 +0100
   From: Martin Husemann mar...@duskware.de

   On Mon, Nov 17, 2014 at 11:48:49PM +0900, Izumi Tsutsui wrote:
You don't think consistently using uint16_t assingments is not necessary.
I think it's necessary to explicitly describe how cksum should be
caluclated and written.  Both are personal opinions, and I don't think
there is a right answer.

   I'm sorry, but I have a hard time following the whole argument - there has
   to be a way to write the code clearly and readable but still avoid undefined
   behaviour (and not paper over it by disabling some optimizations in the
   current compiler).

Can we just copy it to a temporary uint16_t array?  How big are these
boot blocks?  A kilobyte?

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

[Christos Zoulas]

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

| I still don't understand why people don't want one additional
| -fno-strict-aliasing even for Tier-II ports...
| 
http://nxr.netbsd.org/search?q=fno-strict-aliasingproject=srcdefs=refs=path=%2Fsrc%2Fsys

You evolve the code with the language, and you fix things as they come
along. If we did not do that we would still be in the age of KR. Just
because a situation is not perfect we don't aim to make it worse.

christos

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

[Taylor R Campbell]

   Date: Sun, 16 Nov 2014 12:51:34 +0900
   From: Izumi Tsutsui tsut...@ceres.dti.ne.jp

   My understanding is the strict aliasing rule is referred by compiler
   for reorder optimization etc.  If the access via union is still invalid,
   why does the strict gcc48 no longer complain against it?

It's access via two different pointers, which happen to have the same
bits by virtue of being stored in a union -- the union doesn't
actually do anything about aliasing of the pointed-to object, except
probably confuse gcc.

The point is that you can't get at one object in memory through two
pointers of different types (except to get at a non-char object via a
char pointer).

The compiler sometimes tries to detect when you're violating this
rule, but it can't always -- casting through void *, or passing the
pointer through a union of pointer types, may suppress warnings, but
will still violate the rule.

   It's TierII MD code, so maintainability is much more important than
   perfection (unless you will take maintainership of this installboot).

I read the thread, but I'm not clear on how using memcpy in order to
avoid undefined behaviour reduces maintainability.

I don't see any byte ordering issues here that weren't present before.

   Currently it may work.  But once we will try to make the MD installboot
   merged into MI usr.sbin/installboot, accessing variables via different
   types always confuse programmers who don't know actuall intention
   (which is the host endian value, which is the target endian value etc).

Changing `*(uint16_t *)p = v' to `memcpy(p, v, 2)' doesn't change any
of that.  Seems to me if one wants to clarify the intention, one
should add host16enc and target16enc and replace memcpy by whichever
of those is appropriate.

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

[Izumi Tsutsui]

 It's access via two different pointers, which happen to have the same
 bits by virtue of being stored in a union -- the union doesn't
 actually do anything about aliasing of the pointed-to object, except
 probably confuse gcc.
 
 The point is that you can't get at one object in memory through two
 pointers of different types (except to get at a non-char object via a
 char pointer).
 
 The compiler sometimes tries to detect when you're violating this
 rule, but it can't always -- casting through void *, or passing the
 pointer through a union of pointer types, may suppress warnings, but
 will still violate the rule.

Then what's your point? 
Should we always strictly use memcpy even in MD code you won't maintain?

If you really don't like current code, I'd still like to keep
the original casts with -fno-strict-aliasing, instead of memcpy
because my goal is just to switch m68k ports to using gcc48 by default
in netbsd-7, not playing with modern compiler and C specifications.

 Changing `*(uint16_t *)p = v' to `memcpy(p, v, 2)' doesn't change any
 of that.

The formar can be easily changed
 *(uint16_t *)p = htobe16(v);
but the latter can't. You might be able to use functions like
host16enc and target16enc for streaming data, but the target
cksum is not stream but just calculated in uint16_t.

---
Izumi Tsutsui

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

[Christos Zoulas]

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

| Then what's your point? 
| Should we always strictly use memcpy even in MD code you won't maintain?
| 
| If you really don't like current code, I'd still like to keep
| the original casts with -fno-strict-aliasing, instead of memcpy
| because my goal is just to switch m68k ports to using gcc48 by default
| in netbsd-7, not playing with modern compiler and C specifications.

I don't understand why you are so much against the memcpy. This is
what we were already doing with the cast (before it became undefined
behavior) and the language definition mandates it. It is not like
the compiler has to warn about pointer aliasing, or that
-fno-strict-aliasing will work with every compiler... Or even in
the next version of gcc. We fix the warnings to future-proof the code.

| The formar can be easily changed
|  *(uint16_t *)p = htobe16(v);
| but the latter can't. You might be able to use functions like
| host16enc and target16enc for streaming data, but the target
| cksum is not stream but just calculated in uint16_t.

I am sure we can fix the code in a way that it is both readable and correct.
Leaving it the way it was, is not an option.

christos

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

[Taylor R Campbell]

   Date: Mon, 17 Nov 2014 01:37:37 +0900
   From: Izumi Tsutsui tsut...@ceres.dti.ne.jp

Changing `*(uint16_t *)p = v' to `memcpy(p, v, 2)' doesn't change any
of that.

   The formar can be easily changed
*(uint16_t *)p = htobe16(v);
   but the latter can't. You might be able to use functions like
   host16enc and target16enc for streaming data, but the target
   cksum is not stream but just calculated in uint16_t.

So write this?

host16enc(bb-bb_xxboot + 510, 0);
host16enc(bb-bb_xxboot + 510, 0x1234 - abcksum(bb-bb_xxboot));

I don't see the problem.

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

[Christos Zoulas]

On Nov 16,  6:07pm, campbell+netbsd-source-change...@mumble.net (Taylor R 
Campbell) wrote:
-- Subject: Re: CVS commit: src/sys/arch/atari/stand/installboot

|Date: Mon, 17 Nov 2014 01:37:37 +0900
|From: Izumi Tsutsui tsut...@ceres.dti.ne.jp
| 
| Changing `*(uint16_t *)p = v' to `memcpy(p, v, 2)' doesn't change any
| of that.
| 
|The formar can be easily changed
| *(uint16_t *)p = htobe16(v);
|but the latter can't. You might be able to use functions like
|host16enc and target16enc for streaming data, but the target
|cksum is not stream but just calculated in uint16_t.
| 
| So write this?
| 
|   host16enc(bb-bb_xxboot + 510, 0);
|   host16enc(bb-bb_xxboot + 510, 0x1234 - abcksum(bb-bb_xxboot));
| 
| I don't see the problem.

Yes be16enc() should work just fine, I think.

christos

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

[Izumi Tsutsui]

christos@ wrote:

 Yes be16enc() should work just fine, I think.

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

---
/* set AHDI checksum */
sum = 0;
memcpy(bb-bb_xxboot + 255 * sizeof(sum), sum, sizeof(sum));
sum = 0x1234 - abcksum(bb-bb_xxboot);
memcpy(bb-bb_xxboot + 255 * sizeof(sum), sum, sizeof(sum));
 :

static u_int
abcksum (void *bs)
{
u_int16_t sum  = 0,
  *st  = (u_int16_t *)bs,
  *end = (u_int16_t *)bs + 256;

while (st  end)
sum += *st++;
return(sum);
}
---

I think the correct fix is to redefine a boot block structures as
a union of existing struct bootblock and uint16_t array in sys/bootblock.h,
but it would be done when we will merge it into MI installboot.

Fixing only a visible part you are shown in a patch to appease compiler
makes no sense even for correctness.

---
Izumi Tsutsui

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

[Christos Zoulas]

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

| christos@ wrote:
| 
|  Yes be16enc() should work just fine, I think.
| 
| Then why don't you guys also complain to fix existing abcksum() function
| which is called at the suggested memcpy?

Because it is legally converting a void * pointer.

christos

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

[Izumi Tsutsui]

martin@ wrote:

 I still don't see the segmentation violation - what am I missing?
 Gdb is a bit confused about the stack:
 
 (gdb) bt
 #0  0x0008c2b8 in ?? ()
 #1  0xaba4 in ?? ()
 #2  0xbc34 in ?? ()
 #3  0x0006fce8 in ?? ()
 #4  0x in ?? ()

With unstripped binary, it looks distrib/utils/libhack issue?

---
(gdb) run y/0123456789/abcdefghij/
Starting program: 
/r/work/netbsd-7/src/distrib/atari/floppies/install/obj.atari/sed/sed 
y/0123456789/abcdefghij/

Program received signal SIGSEGV, Segmentation fault.
0x0008bbce in mbsrtowcs ()
(gdb) bt
#0  0x0008bbce in mbsrtowcs ()
#1  0x0006f9b2 in compile_stream.clone ()
#2  0x0006fbf2 in compile$$from$$sed ()
#3  0x0006ff28 in _crunched_sed_stub ()
#4  0x21e6 in ___start ()
#5  0x20d4 in _start ()
(gdb)

---
Izumi Tsutsui

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

[Izumi Tsutsui]

I wrote:

 With unstripped binary, it looks distrib/utils/libhack issue?
 
 ---
 (gdb) run y/0123456789/abcdefghij/
 Starting program: 
 /r/work/netbsd-7/src/distrib/atari/floppies/install/obj.atari/sed/sed 
 y/0123456789/abcdefghij/
 
 Program received signal SIGSEGV, Segmentation fault.
 0x0008bbce in mbsrtowcs ()
 (gdb) bt
 #0  0x0008bbce in mbsrtowcs ()
 #1  0x0006f9b2 in compile_stream.clone ()
 #2  0x0006fbf2 in compile$$from$$sed ()
 #3  0x0006ff28 in _crunched_sed_stub ()
 #4  0x21e6 in ___start ()
 #5  0x20d4 in _start ()
 (gdb)

With -g debug binary:
---
(gdb) run y/0123456789/abcdefghij/
Starting program: 
/r/work/netbsd-7/src/distrib/atari/floppies/install/obj.atari/sed/sed 
y/0123456789/abcdefghij/

Program received signal SIGSEGV, Segmentation fault.
0x0007d662 in mbrtowc (ps=0x0, max_sz=1, str=0xffefdc27 0123456789, wc=0x0)
at /r/work/netbsd-7/src/distrib/utils/libhack/multibyte.c:15
15  return str == NULL || (*wc = (unsigned char)*str) == 0 ? 0 : 1;
(gdb) bt
#0  0x0007d662 in mbrtowc (ps=0x0, max_sz=1, str=0xffefdc27 0123456789,
wc=0x0) at /r/work/netbsd-7/src/distrib/utils/libhack/multibyte.c:15
#1  mbsrtowcs (pwcs=0x0, s=0xffefd41c, n=0, ps=0x0)
at /r/work/netbsd-7/src/distrib/utils/libhack/multibyte.c:98
#2  0x00062dee in compile_tr (py=0x10204054, p=0x1bc325 lbuf+24 )
at /r/work/netbsd-7/src/usr.bin/sed/compile.c:676
#3  compile_stream (link=0x10204040)
at /r/work/netbsd-7/src/usr.bin/sed/compile.c:356
#4  0x00063010 in compile () at /r/work/netbsd-7/src/usr.bin/sed/compile.c:144
#5  0x00101a24 in main (argc=optimized out, argv=0xffefecc0)
at /r/work/netbsd-7/src/usr.bin/sed/main.c:207
#6  0x21e6 in ___start ()
#7  0x20d4 in _start ()
(gdb)

---
Izumi Tsutsui

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

[Christos Zoulas]

On Nov 15,  1:43pm, tsut...@ceres.dti.ne.jp (Izumi Tsutsui) wrote:
-- Subject: Re: CVS commit: src/sys/arch/atari/stand/installboot

| How about this one? (cksum seems updated properly on the real machine)

That looks nicer, go for it!

christos
| 
| --- installboot/installboot.c 2014-11-14 13:21:10.0 +0900
| +++ installboot/installboot.c 2014-11-14 23:03:28.0 +0900
| @@ -467,7 +467,10 @@
|  struct disklabel *label, u_int magic)
|  {
|   int  fd;
| - uint16_t sum;
| + union {
| + struct bootblock *bbp;
| + uint16_t *word; /* to fill cksum word */
| + } bbsec;
|  
|   memset(bb, 0, sizeof(*bb));
|  
| @@ -499,10 +502,9 @@
|   setIDEpar(bb-bb_xxboot, sizeof(bb-bb_xxboot));
|  
|   /* set AHDI checksum */
| - sum = 0;
| - memcpy(bb-bb_xxboot + 255 * sizeof(sum), sum, sizeof(sum));
| - sum = 0x1234 - abcksum(bb-bb_xxboot);
| - memcpy(bb-bb_xxboot + 255 * sizeof(sum), sum, sizeof(sum));
| + bbsec.bbp = bb;
| + bbsec.word[255] = 0;
| + bbsec.word[255] = 0x1234 - abcksum(bb-bb_xxboot);
|  
|   if (verbose) {
|   printf(Primary   boot loader: %s\n, xxb);
| 
| ---
| Izumi Tsutsui
-- End of excerpt from Izumi Tsutsui

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

[Christos Zoulas]

On Nov 15, 10:46pm, tsut...@ceres.dti.ne.jp (Izumi Tsutsui) wrote:
-- Subject: Re: CVS commit: src/sys/arch/atari/stand/installboot

| Program received signal SIGSEGV, Segmentation fault.
| 0x0007d662 in mbrtowc (ps=0x0, max_sz=1, str=0xffefdc27 0123456789, wc=0x0)
| at /r/work/netbsd-7/src/distrib/utils/libhack/multibyte.c:15
| 15  return str == NULL || (*wc = (unsigned char)*str) == 0 ? 0 : 
1;

Ok, this is a bug in libhack; please cvs update.

christos

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

[Taylor R Campbell]

   Date: Sat, 15 Nov 2014 13:43:16 +0900
   From: Izumi Tsutsui tsut...@ceres.dti.ne.jp

   -uint16_t sum;
   +union {
   +struct bootblock *bbp;
   +uint16_t *word; /* to fill cksum word */
   +} bbsec;
   ...
   -sum = 0;
   -memcpy(bb-bb_xxboot + 255 * sizeof(sum), sum, sizeof(sum));
   -sum = 0x1234 - abcksum(bb-bb_xxboot);
   -memcpy(bb-bb_xxboot + 255 * sizeof(sum), sum, sizeof(sum));
   +bbsec.bbp = bb;
   +bbsec.word[255] = 0;
   +bbsec.word[255] = 0x1234 - abcksum(bb-bb_xxboot);

Um, that has the same issue as the original code, no?  It still refers
to the content of the struct bootblock object by two different types,
struct bootblock and uint16_t -- passing the pointer through a union
doesn't change that.

What's wrong with the memcpy?

If you don't like the way the memcpy code looks, you could write

set16(bb-bb_xxboot, 255, 0);
set16(bb-bb_xxboot, 255, 0x1234 - abcksum(bb-bb_xxboot));

void
set16(void *p, size_t off, uint16_t v)
{
memcpy((uint8_t *)p + off*sizeof(uint16_t), v, sizeof v);
}

I don't see any byte ordering issues here that weren't present before.

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

[Izumi Tsutsui]

christos@ wrote:

 | Program received signal SIGSEGV, Segmentation fault.
 | 0x0007d662 in mbrtowc (ps=0x0, max_sz=1, str=0xffefdc27 0123456789, 
 wc=0x0)
 | at /r/work/netbsd-7/src/distrib/utils/libhack/multibyte.c:15
 | 15  return str == NULL || (*wc = (unsigned char)*str) == 0 ? 0 
 : 1;
 
 Ok, this is a bug in libhack; please cvs update.

Christos, could you please at least compile before commit?

http://mail-index.netbsd.org/source-changes/2014/11/15/msg060599.html
 Log Message:
 Remove unused variable.

Such botch and additional fixes make pullups annoying...

---
Izumi Tsutsui

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

[Christos Zoulas]

On Nov 16,  1:20pm, tsut...@ceres.dti.ne.jp (Izumi Tsutsui) wrote:
-- Subject: Re: CVS commit: src/sys/arch/atari/stand/installboot

| Christos, could you please at least compile before commit?

I compiled, but not with the right flags (obviously).

| http://mail-index.netbsd.org/source-changes/2014/11/15/msg060599.html
|  Log Message:
|  Remove unused variable.
| 
| Such botch and additional fixes make pullups annoying...

Sorry,

christos

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

[Izumi Tsutsui]

christos@ wrote:

 |  but memcpy is the only way.
 | 
 | - cast via (void *)
 
 That does not work.

Hmm. How about -fno-strict-aliasing?

 | - union {uint16_t w[256]; struct bootblock bbp;}
 
 That works...
 
 | - be16enc(9)
 
 I don't see how that helps...

There are two points:

- The port page says keeping it working is the responsibility of the
  user community for Tier II port, and there is a user who has a real
  machine and said I'll check tomorrow, so you don't have to rush to
  commit fixes you cannot test.

- The MD installboot implementation heavily depends on hardware specific
  features but there are few documents and descriptions about such hardware
  except existing source code, so it's much more important to keep
  intention of the original author in such MD code for future readers
  than appeasing strict modern compilers by mechanical changes.
  Furthermore, unexpected bugs could often slip in by such untested
  mechanical changes and it makes harder to track problems in future.

I'm still trying to get atari booting with HAVE_GCC=48,
but there are still more problems...
(sysinst floppy overflow, sed(1) in the install floppy dumps core, etc)

---
Izumi Tsutsui

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

[Christos Zoulas]

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

| Hmm. How about -fno-strict-aliasing?

Hmm, no :-)

|  
|  That works...
|  
|  | - be16enc(9)
|  
|  I don't see how that helps...
| 
| There are two points:
| 
| - The port page says keeping it working is the responsibility of the
|   user community for Tier II port, and there is a user who has a real
|   machine and said I'll check tomorrow, so you don't have to rush to
|   commit fixes you cannot test.
| 
| - The MD installboot implementation heavily depends on hardware specific
|   features but there are few documents and descriptions about such hardware
|   except existing source code, so it's much more important to keep
|   intention of the original author in such MD code for future readers
|   than appeasing strict modern compilers by mechanical changes.
|   Furthermore, unexpected bugs could often slip in by such untested
|   mechanical changes and it makes harder to track problems in future.

Well, the code should be functionally equivalent and I doubt that anyone
has ever tested the atari installboot in a little endian machine. Or the
vax one on atari, which was the other bug that gcc found and could have
never worked...

| I'm still trying to get atari booting with HAVE_GCC=48,
| but there are still more problems...
| (sysinst floppy overflow, sed(1) in the install floppy dumps core, etc)

How can I help? Is there an emulator?

christos

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

[Izumi Tsutsui]

christos@ wrote:

 Well, the code should be functionally equivalent

There were many examples readability is better for ancient ports.

In most cases, geeks who knows the machines are not familiar
with NetBSD, and NetBSD geeks don't know the hardware behaivor.

 and I doubt that anyone
 has ever tested the atari installboot in a little endian machine. Or the
 vax one on atari, which was the other bug that gcc found and could have
 never worked...

Note sys/arch/atari/stand/installboot is not cross ready and just
a native binary (it's in my TODO list to add atari support into
MI installboot though).  Fortunately installer floppy doesn't require
installboot. (bootstrap kernels are loaded by loadbsd.ttp for Atari TOS)

 | I'm still trying to get atari booting with HAVE_GCC=48,
 | but there are still more problems...
 | (sysinst floppy overflow, sed(1) in the install floppy dumps core, etc)
 
 How can I help? Is there an emulator?

pkgsrc/emulators/aranym can boot FALCON kernel and show boot messages
but it doesn't implement MFP timer required to run NetBSD/atari..

Anyway, it looks even with gcc45 sed(1) in atari's sysinst.fs dumps core.

---
erase ^H, werase ^W, kill ^U, intr ^C
[1]   Broken pipe sysctl -n kern.root_partition |
  Segmentation fault  sed y/0123456789/abcdefghij/
mount: cannot open `/dev/fd0': No such file or directory
Unable to mount /dev/fd0 read-write
erase ^H, werase ^W, kill ^U, intr ^C

 :
---

You can test it on tme by copying crunched /usr/bin/sed binary
from netbsd-7 atari's sysinst.fs.gz:
ftp://nyftp.netbsd.org/pub/NetBSD-daily/netbsd-7/201411140020Z/atari/installation/miniroot/sysinst.fs.gz

./sed y/0123456789/abcdefghij/ dumps core even on TME sun3
(though the installer worked on NetBSD/atari 6.0 days).
Some libc changes trigger it?

---
Izumi Tsutsui

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

[Christos Zoulas]

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

| christos@ wrote:
| 
|  Well, the code should be functionally equivalent
| 
| There were many examples readability is better for ancient ports.
| 
| In most cases, geeks who knows the machines are not familiar
| with NetBSD, and NetBSD geeks don't know the hardware behaivor.
| 
|  and I doubt that anyone
|  has ever tested the atari installboot in a little endian machine. Or the
|  vax one on atari, which was the other bug that gcc found and could have
|  never worked...
| 
| Note sys/arch/atari/stand/installboot is not cross ready and just
| a native binary (it's in my TODO list to add atari support into
| MI installboot though).  Fortunately installer floppy doesn't require
| installboot. (bootstrap kernels are loaded by loadbsd.ttp for Atari TOS)
| 
|  | I'm still trying to get atari booting with HAVE_GCC=48,
|  | but there are still more problems...
|  | (sysinst floppy overflow, sed(1) in the install floppy dumps core, etc)
|  
|  How can I help? Is there an emulator?
| 
| pkgsrc/emulators/aranym can boot FALCON kernel and show boot messages
| but it doesn't implement MFP timer required to run NetBSD/atari..
| 
| Anyway, it looks even with gcc45 sed(1) in atari's sysinst.fs dumps core.
| 
| ---
| erase ^H, werase ^W, kill ^U, intr ^C
| [1]   Broken pipe sysctl -n kern.root_partition |
|   Segmentation fault  sed y/0123456789/abcdefghij/
| mount: cannot open `/dev/fd0': No such file or directory
| Unable to mount /dev/fd0 read-write
| erase ^H, werase ^W, kill ^U, intr ^C
| 
|  :
| ---
| 
| You can test it on tme by copying crunched /usr/bin/sed binary
| from netbsd-7 atari's sysinst.fs.gz:
| 
ftp://nyftp.netbsd.org/pub/NetBSD-daily/netbsd-7/201411140020Z/atari/installation/miniroot/sysinst.fs.gz
| 
| ./sed y/0123456789/abcdefghij/ dumps core even on TME sun3
| (though the installer worked on NetBSD/atari 6.0 days).
| Some libc changes trigger it?

Crap, I trashed my sun3 tme. I will rebuild it

christos

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

[Martin Husemann]

 On Nov 15,  3:59am, tsut...@ceres.dti.ne.jp (Izumi Tsutsui) wrote:
 | You can test it on tme by copying crunched /usr/bin/sed binary
 | from netbsd-7 atari's sysinst.fs.gz:
 | 
 ftp://nyftp.netbsd.org/pub/NetBSD-daily/netbsd-7/201411140020Z/atari/installation/miniroot/sysinst.fs.gz
 | 
 | ./sed y/0123456789/abcdefghij/ dumps core even on TME sun3

Tried on mac68k (gcc 4.8 compiled) and get:

Core was generated by `sed'.
Program terminated with signal SIGSEGV, Segmentation fault.
...
   0x8c2b4: movel %d1,%a1@+
   0x8c2b6: beqs 0x8c2be
= 0x8c2b8: addql #1,%d0
   0x8c2ba: cmpl %d0,%d2
   0x8c2bc: bccs 0x8c2ac
   0x8c2be: movel %sp@+,%d2
   0x8c2c0: unlk %fp
   0x8c2c2: rts

(gdb) info reg
d0 0x0  0
d1 0x30 48
d2 0x0  0
d3 0x420304069218368
d4 0x1cf035 1896501
d5 0x70a18  461336
d6 0x1cf04c 1896524
d7 0xb1ffc  729084
a0 0xa3a4   0xa3a4
a1 0x4  0x4
a2 0x8c296  0x8c296
a3 0x0  0x0
a4 0x70a18  0x70a18
a5 0x1cf035 0x1cf035
fp 0x9b4c   0x9b4c
sp 0x9b48   0x9b48
ps 0x0  [ ]
pc 0x8c2b8  0x8c2b8
fpcontrol  0x0  0
fpstatus   0x0  0
fpiaddr0x0  0x0

I'm confused.

Martin

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

[Martin Husemann]

On Fri, Nov 14, 2014 at 11:03:15PM +0100, Martin Husemann wrote:
 Core was generated by `sed'.
 Program terminated with signal SIGSEGV, Segmentation fault.
 ...
0x8c2b4: movel %d1,%a1@+
0x8c2b6: beqs 0x8c2be
 = 0x8c2b8: addql #1,%d0
0x8c2ba: cmpl %d0,%d2
0x8c2bc: bccs 0x8c2ac
0x8c2be: movel %sp@+,%d2
0x8c2c0: unlk %fp
0x8c2c2: rts

More interesting how it got there:

(gdb) x/16i  0x0006fce0
   0x6fce0: lea 0x8c296,%a2
   0x6fce6: jsr %a2@
   0x6fce8: movel %d0,%d4

indirect function pointer call via %a2 to:

(gdb) x/16i 0x8c296
   0x8c296: linkw %fp,#0
   0x8c29a: movel %d2,%sp@-
   0x8c29c: movel %fp@(16),%d2
   0x8c2a0: moveal %fp@(12),%a0
   0x8c2a4: moveal %a0@,%a0
   0x8c2a6: moveal %fp@(8),%a1
   0x8c2aa: clrl %d0
   0x8c2ac: tstl %a0
   0x8c2ae: beqs 0x8c2be
   0x8c2b0: clrl %d1
   0x8c2b2: moveb %a0@+,%d1
   0x8c2b4: movel %d1,%a1@+
   0x8c2b6: beqs 0x8c2be
= 0x8c2b8: addql #1,%d0


I still don't see the segmentation violation - what am I missing?
Gdb is a bit confused about the stack:

(gdb) bt
#0  0x0008c2b8 in ?? ()
#1  0xaba4 in ?? ()
#2  0xbc34 in ?? ()
#3  0x0006fce8 in ?? ()
#4  0x in ?? ()


Martin

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

[Dennis Ferguson]

On 14 Nov, 2014, at 14:18 , Martin Husemann mar...@duskware.de wrote:
 I still don't see the segmentation violation - what am I missing?
 Gdb is a bit confused about the stack:

This

0x8c2b4: movel %d1,%a1@+

looks a little suspicious.  %a1 seems to be 0x4.

Dennis Ferguson

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

[Izumi Tsutsui]

christos@ wrote:

 On Nov 14,  3:00am, tsut...@ceres.dti.ne.jp (Izumi Tsutsui) wrote:

 | Test it (or call for testers) before commit
 | because installboot could be fatal on install floppy and bootstrap.
 | 
 |  but memcpy is the only way.
 | 
 | - cast via (void *)
 
 That does not work.
 
 | - union {uint16_t w[256]; struct bootblock bbp;}
 
 That works...

How about this one? (cksum seems updated properly on the real machine)

--- installboot/installboot.c   2014-11-14 13:21:10.0 +0900
+++ installboot/installboot.c   2014-11-14 23:03:28.0 +0900
@@ -467,7 +467,10 @@
 struct disklabel *label, u_int magic)
 {
int  fd;
-   uint16_t sum;
+   union {
+   struct bootblock *bbp;
+   uint16_t *word; /* to fill cksum word */
+   } bbsec;
 
memset(bb, 0, sizeof(*bb));
 
@@ -499,10 +502,9 @@
setIDEpar(bb-bb_xxboot, sizeof(bb-bb_xxboot));
 
/* set AHDI checksum */
-   sum = 0;
-   memcpy(bb-bb_xxboot + 255 * sizeof(sum), sum, sizeof(sum));
-   sum = 0x1234 - abcksum(bb-bb_xxboot);
-   memcpy(bb-bb_xxboot + 255 * sizeof(sum), sum, sizeof(sum));
+   bbsec.bbp = bb;
+   bbsec.word[255] = 0;
+   bbsec.word[255] = 0x1234 - abcksum(bb-bb_xxboot);
 
if (verbose) {
printf(Primary   boot loader: %s\n, xxb);

---
Izumi Tsutsui

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

[Izumi Tsutsui]

christos@ wrote:

 Module Name:  src
 Committed By: christos
 Date: Thu Nov 13 17:19:29 UTC 2014
 
 Modified Files:
   src/sys/arch/atari/stand/installboot: installboot.c
 
 Log Message:
 fix strict aliasing violations

 - *((u_int16_t *)bb-bb_xxboot + 255) = 0;
 - *((u_int16_t *)bb-bb_xxboot + 255) = 0x1234 - abcksum(bb-bb_xxboot);
 + sum = 0;
 + memcpy(bb-bb_xxboot + 255, sum, sizeof(sum));
 + sum = 0x1234 - abcksum(bb-bb_xxboot);
 + memcpy(bb-bb_xxboot + 255, sum, sizeof(sum));

I doubt your bb-bb_xxboot + 255 is the same place
as the original (u_int16_t *)bb-bb_xxboot + 255
(the cksum word looks at the end of 512 byte sector).

memcpy(9) looks also awful for readers because it hides endianness..

---
Izumi Tsutsui

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

[Christos Zoulas]

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

| christos@ wrote:
| 
|  Module Name:src
|  Committed By:   christos
|  Date:   Thu Nov 13 17:19:29 UTC 2014
|  
|  Modified Files:
|  src/sys/arch/atari/stand/installboot: installboot.c
|  
|  Log Message:
|  fix strict aliasing violations
| 
|  -   *((u_int16_t *)bb-bb_xxboot + 255) = 0;
|  -   *((u_int16_t *)bb-bb_xxboot + 255) = 0x1234 - abcksum(bb-bb_xxboot);
|  +   sum = 0;
|  +   memcpy(bb-bb_xxboot + 255, sum, sizeof(sum));
|  +   sum = 0x1234 - abcksum(bb-bb_xxboot);
|  +   memcpy(bb-bb_xxboot + 255, sum, sizeof(sum));
| 
| I doubt your bb-bb_xxboot + 255 is the same place
| as the original (u_int16_t *)bb-bb_xxboot + 255
| (the cksum word looks at the end of 512 byte sector).
| 
| memcpy(9) looks also awful for readers because it hides endianness..

Let me fix it, but memcpy is the only way.

christos

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

[Christos Zoulas]

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

| Test it (or call for testers) before commit
| because installboot could be fatal on install floppy and bootstrap.
| 
|  but memcpy is the only way.
| 
| - cast via (void *)

That does not work.

| - union {uint16_t w[256]; struct bootblock bbp;}

That works...

| - be16enc(9)

I don't see how that helps...

christos