[9front] gefs unnamed substructure undefined behavior

[9front] gefs unnamed substructure undefined behavior

[ashley]

I don't think this message got through originally so I'm resending it,
apologies if you got it twice.

----- Forwarded message from ashley <ashley@tilde.institute> -----

Date: Tue, 26 Nov 2024 01:56:09 +0000
From: ashley <ashley@tilde.institute>
To: 9front@9front.org
Subject: gefs unnamed substructure undefined behavior

Hiya.

I believe that struct Limbo causes unspecified behavior in the way
that it is currently used in struct Mount, since both contain a field
named "next", and according to *c(1):

> A structure or union may contain unnamed substructures and
> subunions.  The fields of the substructures or subunions
> can then be used as if they were members of the parent
> structure or union (the resolution of a name conflict is
> unspecified). [...]

In this case we seem to get lucky and the compiler correctly figures out
that every reference to mnt->next refers to Mount's next field and not
Limbo's, but unless I'm missing something this usage is still incorrect.

I would suggest renaming Limbo's next to something more specific, since
it is the more general struct.

what do you think?

--- a/sys/src/cmd/gefs/blk.c
+++ b/sys/src/cmd/gefs/blk.c
@@ -804,7 +804,7 @@
 	while(1){
 		ge = agetl(&fs->epoch);
 		p = agetp(&fs->limbo[ge]);
-		l->next = p;
+		l->nextlim = p;
 		if(acasp(&fs->limbo[ge], p, l)){
 			ainc(&fs->nlimbo);
 			break;
@@ -844,7 +844,7 @@
 	while(fs->bfree == nil)
 		rsleep(&fs->bfreerz);
 	f = fs->bfree;
-	fs->bfree = (Bfree*)f->next;
+	fs->bfree = (Bfree*)f->nextlim;
 	qunlock(&fs->bfreelk);
 
 	f->bp = bp;
@@ -917,7 +917,7 @@
 	asetl(&fs->epoch, (ge+1)%3);
 
 	for(; p != nil; p = n){
-		n = p->next;
+		n = p->nextlim;
 		switch(p->op){
 		case DFtree:
 			free(p);
@@ -937,7 +937,7 @@
 			qe.b = nil;
 			qput(a->sync, qe);
 			qlock(&fs->bfreelk);
-			f->next = fs->bfree;
+			f->nextlim = fs->bfree;
 			fs->bfree = f;
 			rwakeup(&fs->bfreerz);
 			qunlock(&fs->bfreelk);
--- a/sys/src/cmd/gefs/dat.h
+++ b/sys/src/cmd/gefs/dat.h
@@ -335,7 +335,7 @@
 };
 
 struct Limbo {
-	Limbo	*next;
+	Limbo	*nextlim;
 	int	op;
 };
 
--- a/sys/src/cmd/gefs/main.c
+++ b/sys/src/cmd/gefs/main.c
@@ -191,7 +191,7 @@
 	g = nil;
 	for(f = bfbuf; f != bfbuf+fs->cmax; f++){
 		f->bp = Zb;
-		f->next = g;
+		f->nextlim = g;
 		g = f;
 	}
 	fs->bfree = g;

----- End forwarded message -----

Re: [9front] gefs unnamed substructure undefined behavior

[ori]

I think this mostly makes sense; I think that I'd also like to change
the compilers to do make the order well defined, with the following
two rules:

	1. If there is a member in an enclosing struct, that takes
	   priority over the enclosed struct:

		struct Bar {
			int x;
		}
		struct Foo {
			Bar;
			int x; // this one gets selected
		};

	2. If there are sibling structs that share a member,
	   accessing that member is an error.

		struct Baz {
			int x;
		};
		struct Quux { // ok to define
			Baz;
			Bar;
		};

		Quux q;

		q.x; // error, ambiguous member access


Quoth ashley <ashley@tilde.institute>:
> I don't think this message got through originally so I'm resending it,
> apologies if you got it twice.
> 
> ----- Forwarded message from ashley <ashley@tilde.institute> -----
> 
> Date: Tue, 26 Nov 2024 01:56:09 +0000
> From: ashley <ashley@tilde.institute>
> To: 9front@9front.org
> Subject: gefs unnamed substructure undefined behavior
> 
> Hiya.
> 
> I believe that struct Limbo causes unspecified behavior in the way
> that it is currently used in struct Mount, since both contain a field
> named "next", and according to *c(1):
> 
> > A structure or union may contain unnamed substructures and
> > subunions.  The fields of the substructures or subunions
> > can then be used as if they were members of the parent
> > structure or union (the resolution of a name conflict is
> > unspecified). [...]
> 
> In this case we seem to get lucky and the compiler correctly figures out
> that every reference to mnt->next refers to Mount's next field and not
> Limbo's, but unless I'm missing something this usage is still incorrect.
> 
> I would suggest renaming Limbo's next to something more specific, since
> it is the more general struct.
> 
> what do you think?
> 
> --- a/sys/src/cmd/gefs/blk.c
> +++ b/sys/src/cmd/gefs/blk.c
> @@ -804,7 +804,7 @@
>  	while(1){
>  		ge = agetl(&fs->epoch);
>  		p = agetp(&fs->limbo[ge]);
> -		l->next = p;
> +		l->nextlim = p;
>  		if(acasp(&fs->limbo[ge], p, l)){
>  			ainc(&fs->nlimbo);
>  			break;
> @@ -844,7 +844,7 @@
>  	while(fs->bfree == nil)
>  		rsleep(&fs->bfreerz);
>  	f = fs->bfree;
> -	fs->bfree = (Bfree*)f->next;
> +	fs->bfree = (Bfree*)f->nextlim;
>  	qunlock(&fs->bfreelk);
>  
>  	f->bp = bp;
> @@ -917,7 +917,7 @@
>  	asetl(&fs->epoch, (ge+1)%3);
>  
>  	for(; p != nil; p = n){
> -		n = p->next;
> +		n = p->nextlim;
>  		switch(p->op){
>  		case DFtree:
>  			free(p);
> @@ -937,7 +937,7 @@
>  			qe.b = nil;
>  			qput(a->sync, qe);
>  			qlock(&fs->bfreelk);
> -			f->next = fs->bfree;
> +			f->nextlim = fs->bfree;
>  			fs->bfree = f;
>  			rwakeup(&fs->bfreerz);
>  			qunlock(&fs->bfreelk);
> --- a/sys/src/cmd/gefs/dat.h
> +++ b/sys/src/cmd/gefs/dat.h
> @@ -335,7 +335,7 @@
>  };
>  
>  struct Limbo {
> -	Limbo	*next;
> +	Limbo	*nextlim;
>  	int	op;
>  };
>  
> --- a/sys/src/cmd/gefs/main.c
> +++ b/sys/src/cmd/gefs/main.c
> @@ -191,7 +191,7 @@
>  	g = nil;
>  	for(f = bfbuf; f != bfbuf+fs->cmax; f++){
>  		f->bp = Zb;
> -		f->next = g;
> +		f->nextlim = g;
>  		g = f;
>  	}
>  	fs->bfree = g;
> 
> ----- End forwarded message -----

Re: [9front] gefs unnamed substructure undefined behavior

[qwx]

Dec 1, 2024 07:11:08 ori@eigenstate.org:

> I think this mostly makes sense; I think that I'd also like to change
> the compilers to do make the order well defined, with the following
> two rules:
>
>     1. If there is a member in an enclosing struct, that takes
>        priority over the enclosed struct:
>
>         struct Bar {
>             int x;
>         }
>         struct Foo {
>             Bar;
>             int x; // this one gets selected
>         };
>
>     2. If there are sibling structs that share a member,
>        accessing that member is an error.
>
>         struct Baz {
>             int x;
>         };
>         struct Quux { // ok to define
>             Baz;
>             Bar;
>         };
>
>         Quux q;
>
>         q.x; // error, ambiguous member access

I would suggest also emitting a warning at least with the -FTVw flags
pulled from the system mkfiles to discourage the practice in general,
and even documenting the change (minimally) in 2c(1) or an errata or
some other document that ships with the system.  IMHO it would be useful
to even keep one-liners referencing the commits or mailing list
threads for compiler changes and additions in general.  I'll try to
just make one unless there are objections and post it here.

Thanks,
qwx


> Quoth ashley <ashley@tilde.institute>:
>> I don't think this message got through originally so I'm resending it,
>> apologies if you got it twice.
>>
>> ----- Forwarded message from ashley <ashley@tilde.institute> -----
>>
>> Date: Tue, 26 Nov 2024 01:56:09 +0000
>> From: ashley <ashley@tilde.institute>
>> To: 9front@9front.org
>> Subject: gefs unnamed substructure undefined behavior
>>
>> Hiya.
>>
>> I believe that struct Limbo causes unspecified behavior in the way
>> that it is currently used in struct Mount, since both contain a field
>> named "next", and according to *c(1):
>>
>>> A structure or union may contain unnamed substructures and
>>> subunions.  The fields of the substructures or subunions
>>> can then be used as if they were members of the parent
>>> structure or union (the resolution of a name conflict is
>>> unspecified). [...]
>>
>> In this case we seem to get lucky and the compiler correctly figures out
>> that every reference to mnt->next refers to Mount's next field and not
>> Limbo's, but unless I'm missing something this usage is still incorrect.
>>
>> I would suggest renaming Limbo's next to something more specific, since
>> it is the more general struct.
>>
>> what do you think?
>>
>> --- a/sys/src/cmd/gefs/blk.c
>> +++ b/sys/src/cmd/gefs/blk.c
>> @@ -804,7 +804,7 @@
>>     while(1){
>>         ge = agetl(&fs->epoch);
>>         p = agetp(&fs->limbo[ge]);
>> -       l->next = p;
>> +       l->nextlim = p;
>>         if(acasp(&fs->limbo[ge], p, l)){
>>             ainc(&fs->nlimbo);
>>             break;
>> @@ -844,7 +844,7 @@
>>     while(fs->bfree == nil)
>>         rsleep(&fs->bfreerz);
>>     f = fs->bfree;
>> -   fs->bfree = (Bfree*)f->next;
>> +   fs->bfree = (Bfree*)f->nextlim;
>>     qunlock(&fs->bfreelk);
>>
>>     f->bp = bp;
>> @@ -917,7 +917,7 @@
>>     asetl(&fs->epoch, (ge+1)%3);
>>
>>     for(; p != nil; p = n){
>> -       n = p->next;
>> +       n = p->nextlim;
>>         switch(p->op){
>>         case DFtree:
>>             free(p);
>> @@ -937,7 +937,7 @@
>>             qe.b = nil;
>>             qput(a->sync, qe);
>>             qlock(&fs->bfreelk);
>> -           f->next = fs->bfree;
>> +           f->nextlim = fs->bfree;
>>             fs->bfree = f;
>>             rwakeup(&fs->bfreerz);
>>             qunlock(&fs->bfreelk);
>> --- a/sys/src/cmd/gefs/dat.h
>> +++ b/sys/src/cmd/gefs/dat.h
>> @@ -335,7 +335,7 @@
>> };
>>
>> struct Limbo {
>> -   Limbo   *next;
>> +   Limbo   *nextlim;
>>     int op;
>> };
>>
>> --- a/sys/src/cmd/gefs/main.c
>> +++ b/sys/src/cmd/gefs/main.c
>> @@ -191,7 +191,7 @@
>>     g = nil;
>>     for(f = bfbuf; f != bfbuf+fs->cmax; f++){
>>         f->bp = Zb;
>> -       f->next = g;
>> +       f->nextlim = g;
>>         g = f;
>>     }
>>     fs->bfree = g;
>>
>> ----- End forwarded message -----

Re: [9front] gefs unnamed substructure undefined behavior

[ori]

Also, note, I think that the current code already behaves
as described here, though I need to read the code and verfiy.

Quoth qwx@sciops.net:
> Dec 1, 2024 07:11:08 ori@eigenstate.org:
> 
> > I think this mostly makes sense; I think that I'd also like to change
> > the compilers to do make the order well defined, with the following
> > two rules:
> >
> >     1. If there is a member in an enclosing struct, that takes
> >        priority over the enclosed struct:
> >
> >         struct Bar {
> >             int x;
> >         }
> >         struct Foo {
> >             Bar;
> >             int x; // this one gets selected
> >         };
> >
> >     2. If there are sibling structs that share a member,
> >        accessing that member is an error.
> >
> >         struct Baz {
> >             int x;
> >         };
> >         struct Quux { // ok to define
> >             Baz;
> >             Bar;
> >         };
> >
> >         Quux q;
> >
> >         q.x; // error, ambiguous member access
> 
> I would suggest also emitting a warning at least with the -FTVw flags
> pulled from the system mkfiles to discourage the practice in general,
> and even documenting the change (minimally) in 2c(1) or an errata or
> some other document that ships with the system.  IMHO it would be useful
> to even keep one-liners referencing the commits or mailing list
> threads for compiler changes and additions in general.  I'll try to
> just make one unless there are objections and post it here.
> 
> Thanks,
> qwx
> 
> 
> > Quoth ashley <ashley@tilde.institute>:
> >> I don't think this message got through originally so I'm resending it,
> >> apologies if you got it twice.
> >>
> >> ----- Forwarded message from ashley <ashley@tilde.institute> -----
> >>
> >> Date: Tue, 26 Nov 2024 01:56:09 +0000
> >> From: ashley <ashley@tilde.institute>
> >> To: 9front@9front.org
> >> Subject: gefs unnamed substructure undefined behavior
> >>
> >> Hiya.
> >>
> >> I believe that struct Limbo causes unspecified behavior in the way
> >> that it is currently used in struct Mount, since both contain a field
> >> named "next", and according to *c(1):
> >>
> >>> A structure or union may contain unnamed substructures and
> >>> subunions.  The fields of the substructures or subunions
> >>> can then be used as if they were members of the parent
> >>> structure or union (the resolution of a name conflict is
> >>> unspecified). [...]
> >>
> >> In this case we seem to get lucky and the compiler correctly figures out
> >> that every reference to mnt->next refers to Mount's next field and not
> >> Limbo's, but unless I'm missing something this usage is still incorrect.
> >>
> >> I would suggest renaming Limbo's next to something more specific, since
> >> it is the more general struct.
> >>
> >> what do you think?
> >>
> >> --- a/sys/src/cmd/gefs/blk.c
> >> +++ b/sys/src/cmd/gefs/blk.c
> >> @@ -804,7 +804,7 @@
> >>     while(1){
> >>         ge = agetl(&fs->epoch);
> >>         p = agetp(&fs->limbo[ge]);
> >> -       l->next = p;
> >> +       l->nextlim = p;
> >>         if(acasp(&fs->limbo[ge], p, l)){
> >>             ainc(&fs->nlimbo);
> >>             break;
> >> @@ -844,7 +844,7 @@
> >>     while(fs->bfree == nil)
> >>         rsleep(&fs->bfreerz);
> >>     f = fs->bfree;
> >> -   fs->bfree = (Bfree*)f->next;
> >> +   fs->bfree = (Bfree*)f->nextlim;
> >>     qunlock(&fs->bfreelk);
> >>
> >>     f->bp = bp;
> >> @@ -917,7 +917,7 @@
> >>     asetl(&fs->epoch, (ge+1)%3);
> >>
> >>     for(; p != nil; p = n){
> >> -       n = p->next;
> >> +       n = p->nextlim;
> >>         switch(p->op){
> >>         case DFtree:
> >>             free(p);
> >> @@ -937,7 +937,7 @@
> >>             qe.b = nil;
> >>             qput(a->sync, qe);
> >>             qlock(&fs->bfreelk);
> >> -           f->next = fs->bfree;
> >> +           f->nextlim = fs->bfree;
> >>             fs->bfree = f;
> >>             rwakeup(&fs->bfreerz);
> >>             qunlock(&fs->bfreelk);
> >> --- a/sys/src/cmd/gefs/dat.h
> >> +++ b/sys/src/cmd/gefs/dat.h
> >> @@ -335,7 +335,7 @@
> >> };
> >>
> >> struct Limbo {
> >> -   Limbo   *next;
> >> +   Limbo   *nextlim;
> >>     int op;
> >> };
> >>
> >> --- a/sys/src/cmd/gefs/main.c
> >> +++ b/sys/src/cmd/gefs/main.c
> >> @@ -191,7 +191,7 @@
> >>     g = nil;
> >>     for(f = bfbuf; f != bfbuf+fs->cmax; f++){
> >>         f->bp = Zb;
> >> -       f->next = g;
> >> +       f->nextlim = g;
> >>         g = f;
> >>     }
> >>     fs->bfree = g;
> >>
> >> ----- End forwarded message -----
> 

Re: [9front] gefs unnamed substructure undefined behavior

[ashley]

that seems cromulent to me.

I noticed this while trying to port gefs to plan9port/gcc it's just
annoying is all. in any case the man page should reflect the actual
behavior, and I agree that hard errors are far preferable to
unspecified behavior if they're possible to detect statically.

Re: [9front] gefs unnamed substructure undefined behavior

[ori]

For plan9port/gcc, you can add the '-fplan9-extensions' flag,
and things will work for this, I believe.

(FWIW, I've ported to linux in order to use tsan and asan,
but didn't find it worthwhile to maintain the port; I can
try to find some of the hacks I put in place if you're
interested)

Quoth ashley <ashley@tilde.institute>:
> that seems cromulent to me.
> 
> I noticed this while trying to port gefs to plan9port/gcc it's just
> annoying is all. in any case the man page should reflect the actual
> behavior, and I agree that hard errors are far preferable to
> unspecified behavior if they're possible to detect statically.

How's this?

diff 051804186780ef7f3b2f4cb548be1dd0685de5fe uncommitted
--- a/sys/man/1/2c
+++ b/sys/man/1/2c
@@ -232,7 +232,7 @@
 A structure or union may contain unnamed substructures and subunions.
 The fields of the substructures or
 subunions can then be used as if they were members of the parent
-structure or union (the resolution of a name conflict is unspecified).
+structure or union.
 When a pointer to the outer structure or union is used in a context
 that is only legal for the unnamed substructure, the compiler promotes
 the type and adjusts the pointer value to point at the substructure.
@@ -241,6 +241,10 @@
 statement,
 the unnamed structure or union can be explicitly referenced
 by <struct variable>.<tagname>.
+If an enclosed structure or union contains a member with the same name as the enclosing structure,
+the enclosing structure takes priority.
+If an enclosing structure or union contains two structures with the same member,
+accessing the ambiguous member without disambiguation is an error.
 .TP
 \-
 A structure value can be formed with an expression such as

Re: [9front] gefs unnamed substructure undefined behavior

[ashley]

sounds good to me.

Re: [9front] gefs unnamed substructure undefined behavior

[ashley]

> For plan9port/gcc, you can add the '-fplan9-extensions' flag,
> and things will work for this, I believe.

oh thanks! I think it mostly boils down to this and exposing the Avltree
structs (which plan9port doesn't). will give it a short now.

> (FWIW, I've ported to linux in order to use tsan and asan,
> but didn't find it worthwhile to maintain the port; I can
> try to find some of the hacks I put in place if you're
> interested)

Re: [9front] gefs unnamed substructure undefined behavior

[ori]

Quoth ashley <ashley@tilde.institute>:
> > For plan9port/gcc, you can add the '-fplan9-extensions' flag,
> > and things will work for this, I believe.
> 
> oh thanks! I think it mostly boils down to this and exposing the Avltree
> structs (which plan9port doesn't). will give it a short now.
> 
> > (FWIW, I've ported to linux in order to use tsan and asan,
> > but didn't find it worthwhile to maintain the port; I can
> > try to find some of the hacks I put in place if you're
> > interested)

Our avl tree diverged quite a bit from p9p's; it's a single file
library, so to port you're probably best off with copying avl.c,
avl.h, , and then doing a s/<avl.h>/"avl.h".