Re: [perl #36374] [PATCH] segmented context and register memory
Chip Salzenberg
version of the patch from Leo will give me time to catch up }
I've reviewed the patch. I appreciate what you're doing with it, but
there are some issues that will have to be addressed.
1. "union Parrot_Context" is not portable C.
The C standard requires that all structure pointers be bitwise
compatible - same size, layout, etc.[*] It doesn't require that of
other pointers -- specifically char* in this case. So the union:
typedef union Parrot_Context {
struct parrot_regs_t *bp; /* register base pointer */
struct Real_Context *rctx; /* realcontext is at rctx[-1] */
char *as_charp; /* to simplify allocation */
} parrot_context_t;
isn't guaranteed to work like you think it does.
There are two good approaches to what you're trying to do here that I
can recommend.
a. remove the as_charp member and do without it.
b. replace the union with a single pointer of whatever type is
convenient ... a structure or a void* or whatever ... and just
cast the pointer as needed. This isn't C++ ... pointer casts
are guaranteed to have zero runtime cost. It's just a macro.
I recommend (b).
2. Naming of "Real_Context".
"Real_Context" is not an OK name. I'm going to go into detail here
because there's a principle involved:
There's no reason ever to name something "real_foo" (except in the
mathematical real-number sense). Because that means something else is
a fake unreal foo, but it's not labelled such. If the "foo" becomes
fake, then it should be labelled so.
If there's a real foo and an unreal foo, then the real one should be
named "foo" and the unreal one should be named "fake_foo" or whatever.
3. REAL_CTX() must be renamed for the reasons given above. I think
CONTEXT() is a good name, but CTX() will do, if only because we could
very well going to have to change the name of "context" to something
more specific one of these days.
4. CTX_VAR(p,m) is a bad idea and should be simply removed. Usage of
CTX_VAR(p,m) should be replaced with CTX(p)->m. Remember code should
be greppable; it should be possible to find most (if not all) member
usage by grepping for ".member" and "->member".
5. FIRST_CTX_VAR is at least unnecessary, and might be a bug.
If you want to zero a structure, why go to the trouble of asking for
the address of its first member? Just use the address of the structure
as a whole. That's the real starting point for sizeof() anyway.
Even worse, I'm not sure the usage of FIRST_CTX_VAR is sound WRT the C
standard(s). Does anyone on the list know, given:
typedef struct {
whatever_t m;
} S;
... whether offsetof(S,m) is guaranteed to be zero? I think it's not,
and so the usage of FIRST_CTX_VAR for e.g. memset is not OK.
6. As long as we have the "parrot_context_t" typedef, we should use it
instead of "struct" or, now, "union Parrot_Context". No point in
hiding the difference between unions and structs under a nice typedef
wrapper if we're just going to rip off the wrapper ourselves.
{ and I stop here }
--
Chip Salzenberg <ch...@pobox.com>
Andy Dougherty
> 1. "union Parrot_Context" is not portable C.
>
> The C standard requires that all structure pointers be bitwise
> compatible - same size, layout, etc.[*] It doesn't require that of
> other pointers -- specifically char* in this case. So the union:
>
> typedef union Parrot_Context {
> struct parrot_regs_t *bp; /* register base pointer */
> struct Real_Context *rctx; /* realcontext is at rctx[-1] */
> char *as_charp; /* to simplify allocation */
> } parrot_context_t;
>
> isn't guaranteed to work like you think it does.
>
> There are two good approaches to what you're trying to do here that I
> can recommend.
>
> a. remove the as_charp member and do without it.
>
> b. replace the union with a single pointer of whatever type is
> convenient ... a structure or a void* or whatever ... and just
> cast the pointer as needed. This isn't C++ ... pointer casts
> are guaranteed to have zero runtime cost. It's just a macro.
>
> I recommend (b).
Well, I think there are already way too many pointer casts and related
games in the source. Perhaps more to the point, not all casts are going
to work.
In particular, the last time I checked with gcc on SPARC, I got over 7,000
warnings of the form 'cast increases required alignment of target type'.
At that time, one of them was leading to a core dump (which I have since
fixed). Assigning to pointers of the correct type automatically documents
what you are actually trying to do, gets rid of the warnings, and assures
that objects are indeed suitably aligned.
In the perl5 source, I can at least look at the ?v.h header files and see
which structures are supposed to match up with each other, and hence I can
deduce which casts are intended. I haven't yet figured out in the parrot
source which structures are supposed to be "equivalent".
> If you want to zero a structure, why go to the trouble of asking for
> the address of its first member? Just use the address of the structure
> as a whole. That's the real starting point for sizeof() anyway.
>
> Even worse, I'm not sure the usage of FIRST_CTX_VAR is sound WRT the C
> standard(s). Does anyone on the list know, given:
>
> typedef struct {
> whatever_t m;
> } S;
>
> ... whether offsetof(S,m) is guaranteed to be zero? I think it's not,
> and so the usage of FIRST_CTX_VAR for e.g. memset is not OK.
I don't know what the standard says about the specific case of the
first element in a structure. In general, of course, the compiler is
free to put in whatever padding it it deems appropriate, but it may be
that the first element is special and can't have padding before it.
More importantly, though I wonder if that particular micro-optimization
is important to do at this point in parrot's development.
--
Andy Dougherty doug...@lafayette.edu
Chip Salzenberg
> Well, I think there are already way too many pointer casts and related
> games in the source. Perhaps more to the point, not all casts are going
> to work.
Well, there is that. :-)
In this case, as I understand it, we're looking at one pointer where
some data are above the pointer and other data are below it. (I'm
basing this on my memory of a design discussion in Austria; if I'm
mistaken, then ignore the rest.)
Given "struct below" and "struct above", this should be portable C:
struct group {
struct below b;
struct above a;
};
struct group *g = malloc(sizeof *g) /* OR WHATEVER METHOD OF ALLOC */
/* now take an 'above' pointer, so access to 'a' is fast & easy */
struct above *a = &g->a; /* addr is (char*)g + offsetof(struct group, a) */
/* now make macros that easily let us point from the 'above' to the 'below' */
#define GROUP(a) (struct group *)((char *)(a) - offsetof(struct group, a))
#define BELOW(a) (&GROUP(a)->b)
struct below *b = B(a);
Look, Ma! No unions! And no worries about alignment, either.
(Of course if this were C++ we could just do multiple inheritance and
let the static_cast<> template handle it. But whatever. :-))
> In particular, the last time I checked with gcc on SPARC, I got over 7,000
> warnings of the form 'cast increases required alignment of target type'.
Then gcc should just shut up, IMO. When I cast from void* or char* to
whatever*, it's my lookout as programmer if that's an error, and
there's nothing about it that deserves a compiler warning ... mostly
because there are some things in C that require it, and the compiler
can't possibly distinguish the good from the bogus. It's like warning
about '=' in boolean expressions. Yeah, I know. :-P
> More importantly, though I wonder if that particular micro-optimization
> is important to do at this point in parrot's development.
Probably not. It's more important pedagogically. We[*] are learning
about C by running into what doesn't work.
[*] not the editorial 'we' this time
--
Chip Salzenberg <ch...@pobox.com>
Leopold Toetsch
> { This is a partial reply; what with YAPC I didn't finish, but a new
> version of the patch from Leo will give me time to catch up }
>
> I've reviewed the patch. I appreciate what you're doing with it, but
> there are some issues that will have to be addressed.
Fixed. See attached file.
> There are two good approaches to what you're trying to do here that I
> can recommend.
>
> a. remove the as_charp member and do without it.
Done a.
gdb> p interp->ctx.rctx[-1]
is a lot simpler in the absence of casts.
leo
Andrew Dougherty
> Chip Salzenberg wrote:
> > { This is a partial reply; what with YAPC I didn't finish, but a new
> > version of the patch from Leo will give me time to catch up }
> >
> > I've reviewed the patch. I appreciate what you're doing with it, but
> > there are some issues that will have to be addressed.
>
> Fixed. See attached file. [context_4.patch]
Unfortunately, it doesn't work for me on Solaris 8/SPARC with Sun's cc.
First, the trivial problem: Sun's compiler complained about the label
at the end of a block in classes/sub.pmc:
"classes/sub.pmc", line 566: syntax error before or at: }
The lines in question are:
is_tail_call:
}
So I fixed that. Then, 'make test' reported more than the usual number of errors:
Failed 7/157 test scripts, 95.54% okay. 22/2625 subtests failed, 99.16% okay.
Failed Test Stat Wstat Total Fail Failed List of Failed
-------------------------------------------------------------------------------
imcc/t/syn/tail.t 1 256 6 1 16.67% 6
t/dynclass/gdbmhash.t 13 3328 13 13 100.00% 1-13
t/p6rules/builtins.t 1 256 10 1 10.00% 10
t/p6rules/capture.t 4 1024 38 4 10.53% 34-37
t/pmc/mmd.t 1 256 28 1 3.57% 25
t/pmc/object-meths.t 1 256 28 1 3.57% 28
t/src/hash.t 1 256 10 1 10.00% 6
4 tests and 74 subtests skipped.
*** Error code 11
Here's one of the failing tests under the debugger: imcc/t/syn/tail_6.pir:
(dbx) run tail_6.pir
Running: miniparrot tail_6.pir
(process id 20346)
t@1 (l@1) signal SEGV (no mapping at the fault address) in Parrot_tailcallmethod_sc at 0xa65cc
0x000a65cc: Parrot_tailcallmethod_sc+0x0050: ld [%o3 + 0x6c], %o3
(dbx) where
current thread: t@1
=>[1] Parrot_tailcallmethod_sc(0x379db8, 0x1d3480, 0x3a8270, 0x3a8268, 0x2286b8, 0x5), at 0xa65cc
[2] runops_slow_core(0x3a8268, 0x3a8118, 0x1d3480, 0x1cc418, 0x1cc414, 0x1f3d70), at 0xeb65c
[3] runops_int(0x1d3480, 0xeb700, 0xeb5d0, 0xeb56c, 0xe81d4, 0xe8164), at 0xe8534
[4] runops(0x1d3480, 0x0, 0x4, 0x3d7f10, 0x228850, 0x0), at 0xe99c4
[5] Parrot_runcode(0x1d3480, 0x1, 0x71c00, 0x228808, 0x394d00, 0x1), at 0x7283c
[6] Parrot_runcode(0x1d3480, 0x1, 0xffbefa80, 0x10, 0xffbefa80, 0x4), at 0x724b0
[7] main(0x1d3480, 0x1cbc00, 0x1cbc00, 0x1cbc00, 0x17bc00, 0x0), at 0x4b7d0
And here's the output with -t:
$ ./miniparrot -t tail_6.pir
0 newclass P30, "Foo" - P30=PMCNULL,
3 addattribute P30, "n" - P30=Class=Foo:PMC(0x2287d8),
6 new P16, "Foo" - P16=PMCNULL,
9 new P15, 31 - P15=PMCNULL,
12 set P15, 2000 - P15=Integer=PMC(0x2286a0: 0),
15 setattribute P16, "Foo\0n", P15 - P16=Object(Foo)=PMC(0x2286b8), , P15=Integer=PMC(0x2286a0: 2000)
19 set I0, 1 - I0=0,
22 set I1, 0 - I1=0,
25 set I2, 0 - I2=0,
28 set I3, 0 - I3=1,
31 set I4, 0 - I4=0,
34 set S1, "O" - ,
37 set P2, P16 - P2=PMCNULL, P16=Object(Foo)=PMC(0x2286b8)
40 callmethodcc "go"
51 interpinfo P14, 16 - P14=PMCNULL,
54 getattribute P15, P14, "Foo\0n" - P15=PMCNULL, P14=Object(Foo)=PMC(0x2286b8),
58 dec P15 - P15=Integer=PMC(0x2286a0: 2000)
60 unless P15, 26 - P15=Integer=PMC(0x2286a0: 1999),
63 set I0, 1 - I0=1,
66 set I1, 0 - I1=0,
69 set I2, 0 - I2=0,
72 set I3, 0 - I3=0,
75 set I4, 0 - I4=0,
78 set S1, "O" - ,
81 set P2, P14 - P2=PMCNULL, P14=Object(Foo)=PMC(0x2286b8)
84 tailcallmethod "go"
Segmentation Fault(coredump)
Alas, I have no idea where to go from here.
--
Andy Dougherty doug...@lafayette.edu
Nicholas Clark
> On Mon, Jun 27, 2005 at 05:11:10PM -0400, Andy Dougherty wrote:
> > Well, I think there are already way too many pointer casts and related
> > games in the source. Perhaps more to the point, not all casts are going
> > to work.
>
> Well, there is that. :-)
> struct group *g = malloc(sizeof *g) /* OR WHATEVER METHOD OF ALLOC */
^typo
> Then gcc should just shut up, IMO. When I cast from void* or char* to
> whatever*, it's my lookout as programmer if that's an error, and
> there's nothing about it that deserves a compiler warning ... mostly
> because there are some things in C that require it, and the compiler
> can't possibly distinguish the good from the bogus. It's like warning
> about '=' in boolean expressions. Yeah, I know. :-P
We may well have the warning because we turned on as many bitchy flags in gcc
as possible.
Do all our casts avoid creating aliasing problems?
Nicholas Clark
Leopold Toetsch
On Jun 30, 2005, at 21:30, Andrew Dougherty wrote:
>
> Failed 7/157 test scripts, 95.54% okay. 22/2625 subtests failed,
> 99.16% okay.
> Failed Test Stat Wstat Total Fail Failed List of Failed
Thanks for trying it out and testing it. I've found hopefully a lot of
these bug. The one tail-call did just use an uninitialized object.
I'll send an updated patch probably tomorrow.
Thanks,
leo