Re: CVS commit: src/sys/arch/atari/stand/installboot
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
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
>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
Date: Mon, 24 Nov 2014 23:50:12 +0900 From: 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.
Re: CVS commit: src/sys/arch/atari/stand/installboot
> 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
Date: Mon, 24 Nov 2014 22:39:24 +0900 From: Izumi Tsutsui 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
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
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
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? Nick
re: CVS commit: src/sys/arch/atari/stand/installboot
> 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
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
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
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
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
Date: Mon, 24 Nov 2014 04:46:31 +0900 From: Izumi Tsutsui 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@ 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
>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
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
Date: Mon, 24 Nov 2014 03:20:27 +0900 From: 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?' 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@ 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
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
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
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
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
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@ 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
> | -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
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
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
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-aliasing&project=src&defs=&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 K&R. 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
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-aliasing&project=src&defs=&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 K&R. 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
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-aliasing&project=src&defs=&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 K&R. 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
martin@ wrote: > (and not paper over it by disabling some optimizations in the > current compiler). 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-aliasing&project=src&defs=&refs=&path=%2Fsrc%2Fsys --- Izumi Tsutsui
Re: CVS commit: src/sys/arch/atari/stand/installboot
Date: Mon, 17 Nov 2014 15:55:56 +0100 From: 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). 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
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
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
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
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
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
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 , 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
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 | |> 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
Date: Mon, 17 Nov 2014 01:37:37 +0900 From: Izumi Tsutsui > 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
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
> 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
Date: Sun, 16 Nov 2014 12:51:34 +0900 From: Izumi Tsutsui 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
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
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
>- 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. IIUC, union is valid in this case per C9899:201x 6.5 Expressions: --- 7 An object shall have its stored value accessed only by an lvalue expression that has one of the following types: : * an aggregate or union type that includes one of the aforementioned types among its members (including, recursively, a member of a subaggregate or contained union), --- 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? > What's wrong with the memcpy? Did you read whole this thread? See http://mail-index.netbsd.org/source-changes-d/2014/11/14/msg007328.html >> - 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. It's TierII MD code, so maintainability is much more important than perfection (unless you will take maintainership of this installboot). > 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). --- Izumi Tsutsui
Re: CVS commit: src/sys/arch/atari/stand/installboot
Date: Sat, 15 Nov 2014 13:43:16 +0900 From: Izumi Tsutsui -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
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
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
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 "") 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=, 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
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
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
On 14 Nov, 2014, at 14:18 , Martin Husemann 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
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
> 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
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
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
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
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
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
Re: CVS commit: src/sys/arch/atari/stand/installboot
christos@ wrote: > 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, "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 *) - union {uint16_t w[256]; struct bootblock bbp;} - be16enc(9) etc? --- Izumi Tsutsui
Re: CVS commit: src/sys/arch/atari/stand/installboot
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@ 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