ptr_alignment 8 not honored, sparc 64bit broken

[rurban]

Sparc 64bit with strict ptr_alignment=8 breaks when reading frozen pmc's because of misalignment.

Any 64bit big-endian cpu with strict alignment is broken without -xmemalign=4s (the immediate workaround) The default for all v9 architectures is -xmemalign=8s.

However, our goal should be to allow fast aligned code and not to skip this with a compiler relaxement.

t@1 (l@1) stopped in PF_fetch_integer at line 1076 in file "pf_items.c"
 1076       ASSERT_ARGS(PF_fetch_integer)
(dbx) step
t@1 (l@1) stopped in PF_fetch_integer at line 1078 in file "pf_items.c"
 1078       if (!pf || pf->fetch_iv == NULL)
(dbx) print *(*stream)
**stream = 4
(dbx) step
t@1 (l@1) stopped in PF_fetch_integer at line 1079 in file "pf_items.c"
 1079           return *(*stream)++;
(dbx) print *(*stream) 
**stream = 4
(dbx) step
t@1 (l@1) signal BUS (invalid address alignment) in PF_fetch_integer at line 1079 in file "pf_items.c"
 1079           return *(*stream)++;
(dbx) print *(*stream) 
**stream = 35
(dbx) print *stream
*stream = 0x100000dec

0x100000dec is not properly aligned. It must be 0x100000df0.

Sparc cc manpage:
-xmemalign[=<a><b>] Controls memory alignment, <a>={1|2|4|8|16}, b={f|i|s}.
Accepted values for b are:
i Interpret access and continue execution.
s Raise signal SIGBUS.
f For variants of -xarch=v9 only. [reduced i]

Thanks to Rolf Grossmann for coming up with all this info and debugging, and to Andy Dougherty for correcting my wrong first analysis.

The fix for 1.0 will be a hints file update to add -xmemalign=4s to cc_flags, the goal for 2.6 will be a --64bitcompat argument to parrot to create 8-byte aligned values on 32-bit, not 4-byte as now.

[doughera]

Replying to rurban:

A strict 64bit cpu with a ptr_alignment=8 will break when reading pbc's or just frozen pmc's because our alignment when writing our bytecode is 16/ptrsize and not 16.

No. You are mixing up units here. The alignment is supposed to be on a 16-byte boundary. However, the code is supposed to step through the pbc in steps of size sizeof(opcode_t). Therefore, it is also true that the alignment is 16/sizeof(opcode_t), when measured in units of sizeof(opcode_t).

As I have explained before, the ALIGN_16 macro assumes you are stepping through the bytecode in steps of sizeof(opcode_t). If there are alignment problems, it seems likely to me that those problems are due to breaking that assumption earlier. The problem is not the ALIGN_16 macro (though it could, of course, be replaced by a more defensive, but slower function), but in the steps preceeding the call to ALIGN_16 which are somehow violating the assumption that the reader is stepping through the file in units of sizeof(opcode_t). For this problem here, a backtrace showing where the stream pointer came from would be helpful.

The decision to step through the file in units of sizeof(opcode_t) was a deliberate choice for at least two reasons: efficiency, and avoiding compiler warnings. gcc on SPARC in particular will issue warnings when casting from (char *) to things like (opcode_t *): "warning: cast increases required alignment of target type". Please don't change that.

For reading foreign files with a different sizeof(opcode_t), you are correct that you will have to step through the file in steps of the foreign stepsize. But that's not the problem here -- everything is native.

[rurban]

I analyzed this further, but cannot test this. See  http://use.perl.org/~rurban/journal/38522

I believe 8-byte ptr_alignment cannot be supported as now, because we do not enforce strict pairs of 4-byte data, as integer and opcodes. There can be an uneven number, and then we get a SIGBUS.

We enforce 16-byte aligment at the start and end of segments and at end of strings, but not with integer and opcodes.

So we should update the Sparc64 hints to use -xmemalign=4i, this is the best we can guarantee (untested).

For the future we should discuss enforcing writing pairs of 4-byte data or padding uneven ending 4-byte data with a pad of one byte.

It should also be tested if the pf_items (unsigned char*)stream++ are valid with Sparc64.

[rg]

Sorry you patch doesn't apply. It seems the debug output part has already been committed but in a different way. Can you check?

I've just testet -xmemalign=4s (we want a fault if we're not correct, not emulation) and it works and passes the tests (as far as solaris passes the tests at all ;)). The native_pbc tests are still TODO, however (and they don't pass ... but then they don't pass on amd64 either).

If you could be more specific what I should test, I'd be happy to. On the other hand, it might not be worth all the effort and we'd rather just update the hints file and fix some problems that don't have a known work-around.

[rurban]

rg: We will stay with -xmemalign=4s until pbc will get fully portable.

The current goal is version 2.6 but I hope we will get it sooner. I already know why it failed. See  http://use.perl.org/~rurban/journal/38522

I also saw that my latest patch was wrong. I will update it soon. But first I will try the hints patch and would like you to test that first, so that

'perl Configure.pl --m=64'

works without any hazzle.

[rurban]

warn also at alignment check

[rurban]

tt364-sparc-memalign.patch applied as r37130.

I'll open another ticket for the optional cmdline option to parrot (--64compat) to produce 8-byte packfiles on 4-byte archs.

[doughera]

The patch to the hints/solaris.pm file in r37130 won't work. The callback is triggering when gccversion is determined. Unfortunately, at that time, byteorder hasn't been determined yet, so the byteorder eq '87654321' condition will never be true. I'll see if I can come up with a patch later today or tomorrow.

[doughera]

Replying to doughera:

I'll see if I can come up with a patch later today or tomorrow.

Unfortunately, I lost way too much time today fighting other bugs and didn't get back to this. I'm not sure if I'll have a chance to do anything before the 1.0 release. Meanwhile, the hints/solaris.pm part of r37130 should probably be reverted, since all it does is cause a spurious warning: 'Use of uninitialized value in string eq at config/init/hints/solaris.pm line 113'.

[doughera]

I think the attached patch (pf_items.p4) will stop the immediate core dump observed in the original bug report. (It may, of course, just move it to somewhere else!)

Could you please try this patch without the -xmemalign=4s compiler flag and report back? If it fails, could you please report on exactly what command failed? Also, could you please attach the 'myconfig' file generated by Configure.pl?

It might make sense to try running just the core tests with perl t/harness --core-tests to see if there's a very simple pir test file that triggers the bug.

Thanks.

[rg]

So I wanted to check out that patch, however to effectively verify that it fixes anything, I've rebuilt parrot without -xmemalign=4s and lo and behold, it worked this time. ( Verification) So something between now and the time we discovered the problem has changed, so that the problem is either fixed or not just not triggered. Obviously this makes it very hard to determine if the patch is still required. Just from reading it, I'm inclined to say it shouldn't be required. Since I guess my vote doesn't count for much, I'm going to let others decide whether to close. I am however making this much less important.

Also, +1 on reverting r37130. It doesn't work and currently doesn't seem neccessary anymore, either. But a big thanks to rurban for the effort.

[rurban]

Andy, It's not just fetch_integer which might get hit by a 4byte alignment.

It is every PF_fetch_* function, since we guarantee only 4-byte alignment when writing 32bit pbc's. Even if it works on sparc64 on the tested pbc's I would not feel safe without changing all fetchers.

But it's definitely worth a good test.

[doughera]

Yes, I know that. But fetch_integer is the place where this particular core dump was reported, so fetch_integer is what I proposed fixing first. It also has the advantage that it's only called from one place in the entire parrot tree, so it's an easily localizable, nicely contained change, worth testing.

If it helped avoid this core dump, I fully expected the core dump to be shifted to a fetch_opcode call. Then it would make sense to think about expanding this strategy to fetch_opcode as well.

I think a patch like this is essentially unavoidable if parrot is going to read foreign pbc files from the same set of functions used to read native pbc files. We can't step ahead by 1/2 an opcode_t.