Ticket #254 (closed bug: fixed)

Opened 5 years ago

Last modified 5 years ago

pbc compat: t/native_pbc/number.t: New failure on some platforms

Reported by: jkeenan Owned by: jkeenan
Priority: normal Milestone:
Component: core Version:
Severity: high Keywords: 64bit
Cc: Language:
Patch status: applied Platform: all

Description

A change today in t/native_pbc/number.t (and possibly other files) has caused a test in that file to begin to fail on some, but not all, platforms.

See attachments.

I experienced this failure on Darwin/PPC, but not on Linux/i386. It was also reported on x86_64 on ICC.

Attachments

t.native_pbc.number.t.failure.txt Download (1.0 KB) - added by jkeenan 5 years ago.
prove -v t/native_pbc/number.t (Darwin)
t.native_pbc.number.t.modifications.txt Download (2.2 KB) - added by jkeenan 5 years ago.
t/native_pbc/number.t: Revisions that may have caused test failure
tt254-floattype1-bug.patch Download (1.1 KB) - added by rurban 5 years ago.
replaces first version from nopaste
tt254-64bit-pbc.tar.gz Download (15.4 KB) - added by rurban 5 years ago.
64bit patch + 2 new files
some_native_pbc.tar.gz Download (2.0 KB) - added by rg 5 years ago.
native pbc _4 and _6 (64bit)
tt254-64bit-pbc.patch Download (49.9 KB) - added by rurban 5 years ago.
native_pbc-number.t-failure.txt Download (1.3 KB) - added by doughera 5 years ago.
myconfig-opensolaris.txt Download (1.0 KB) - added by doughera 5 years ago.
tt254-64bit-2.patch Download (12.9 KB) - added by rurban 5 years ago.
This fixes now the 64bit issues. one major, one minor bug
tt254-64bit-3.patch Download (12.3 KB) - added by NotFound 5 years ago.
remove-alignment-warning.patch Download (1.0 KB) - added by doughera 5 years ago.
remove-alignment-warning.adjusted.patch Download (1.0 KB) - added by jkeenan 5 years ago.
Andy D's patch adjusted for c_parens coding standard

Change History

Changed 5 years ago by jkeenan

prove -v t/native_pbc/number.t (Darwin)

Changed 5 years ago by jkeenan

t/native_pbc/number.t: Revisions that may have caused test failure

  Changed 5 years ago by jkeenan

This failure was reported on x86_64 with ICC in a different file in the same directory:

perl t/harness --gc-debug --running-make-test t/native_pbc/integer.t
t/native_pbc/integer....
t/native_pbc/integer....NOK 1/1#   Failed test 'i386 32 bit opcode_t, 32 bit intval'
#   at t/native_pbc/integer.t line 57.
# Exited with error code: 1
# Received:
# PackFile_unpack segment 'BYTECODE_i1.pasm' directory length 0 length in file 0 needed 4 for unpack
# PackFile_unpack segment 'FIXUP_i1.pasm' directory length 3 length in file 3 needed 4 for unpack
# Constant_unpack: Unrecognized type '' during unpack!
# PackFile_unpack segment 'CONSTANT_i1.pasm' failed
# Subroutine returned a NULL address
# current instr.: '(null)' pc -1 ((unknown file):-1)
# 
# Expected:
# 270544960
# May need to regenerate t/native_pbc/integer_1.pbc; read test file
# Looks like you failed 1 test of 1.
t/native_pbc/integer....dubious                                              
        Test returned status 1 (wstat 256, 0x100)
DIED. FAILED test 1
        Failed 1/1 tests, 0.00% okay
Failed Test            Stat Wstat Total Fail  List of Failed
-------------------------------------------------------------------------------
t/native_pbc/integer.t    1   256     1    1  1
Failed 1/1 test scripts. 1/1 subtests failed.
Files=1, Tests=1,  0 wallclock secs ( 0.14 cusr +  0.06 csys =  0.20 CPU)
Failed 1/1 test programs. 1/1 subtests failed.

This file also underwent revisions today.

$ svn diff -r {2009-01-27} t/native_pbc/integer.t 
Index: t/native_pbc/integer.t
===================================================================
--- t/native_pbc/integer.t      (revision 36042)
+++ t/native_pbc/integer.t      (working copy)
@@ -7,7 +7,7 @@
 use lib qw( . lib ../lib ../../lib );
 use Test::More;
 
-use Parrot::Test skip_all => 'ongoing PBC format changes';
+use Parrot::Test tests => 1;
 
 =head1 NAME
 
@@ -25,7 +25,7 @@
 
 =begin comment
 
-s. t/native_pbc/number.t for additional comments
+see t/native_pbc/number.t for additional comments
 
 Test files on different architectures are generated by:
 
@@ -46,10 +46,11 @@
 # execute the file t/native_pbc/integer_1.pbc
 #
 # HEADER => [
-#         wordsize  = 4   (interpreter's wordsize    = 4)
-#         int_size  = 4   (interpreter's INTVAL size = 4)
-#         byteorder = 0   (interpreter's byteorder   = 0)
-#         floattype = 0   (interpreter's NUMVAL_SIZE = 8)
+#         wordsize  = 4   (interpreter's wordsize/INTVAL = 4/4)
+#         byteorder = 0   (interpreter's byteorder       = 0)
+#         floattype = 0   (interpreter's NUMVAL_SIZE     = 8)
+#         parrot-version 0.9.0, bytecode-version 3.34
+#         UUID type = 0, UUID size = 0
 #         no endianize, no opcode, no numval transform
 #         dirformat = 1
 # ]
@@ -57,9 +58,9 @@
     or diag "May need to regenerate t/native_pbc/integer_1.pbc; read test file";
 
 # Formerly following tests had been set up:
-# pasm_output_is(undef, '270544960', "PPC BE 32 bit opcode_t, 32 bit intval");
-# pasm_output_is(undef, '270544960', "little-endian 64-bit tru64");
-# pasm_output_is(undef, '270544960', "big-endian 64-bit irix");
+# pbc_output_is(undef, '270544960', "PPC BE 32 bit opcode_t, 32 bit intval");
+# pbc_output_is(undef, '270544960', "little-endian 64-bit tru64");
+# pbc_output_is(undef, '270544960', "big-endian 64-bit irix");
 
 # Local Variables:
 #   mode: cperl

Here is the log message for that revision:

------------------------------------------------------------------------
r36160 | rurban | 2009-01-29 16:09:59 -0500 (Thu, 29 Jan 2009) | 7 lines

[test] native_pbc:
- Adapt number and integer to new pbc format, string not yet
- Create new reference pbc's for i386 32 bit opcode_t, 32 bit intval
- Will need other platforms pbc's.

I should note that this test did not fail for me on Darwin/PPC.

follow-up: ↓ 5   Changed 5 years ago by rurban

  • status changed from new to assigned
  • owner set to rurban
  • platform set to darwin

Thanks, pbc's must be portable, that's why I enabled the tests. I can work on x86_64 with ICC, but not on Darwin/PPC.

What is the output of Darwin/PCC for ./pdump -h n.pbc ?

$ ./parrot -o n.pbc t/op/number_1.pasm $ make pdump $ ./pdump -h n.pbc

  Changed 5 years ago by rurban

  • component changed from none to core
  • severity changed from medium to high

  Changed 5 years ago by coke

Let's todo them on platforms they are known to fail on, so that we can get our smolder to run clean.

in reply to: ↑ 2 ; follow-up: ↓ 6   Changed 5 years ago by jkeenan

Replying to rurban:

Thanks, pbc's must be portable, that's why I enabled the tests. I can work on x86_64 with ICC, but not on Darwin/PPC. What is the output of Darwin/PCC for ./pdump -h n.pbc ? $ ./parrot -o n.pbc t/op/number_1.pasm $ make pdump $ ./pdump -h n.pbc

Reini: Thanks for investigating this:

[parrot] 512 $ ./parrot -o n.pbc t/op/number_1.pasm
[parrot] 513 $ make pdump
src/pdump.c
src/packdump.c
/usr/bin/g++ -o pdump \
    src/pdump.o \
    src/packdump.o -L/topdir/blib/lib -L/topdir/blib/lib -lparrot  -lm -lgmp -lreadline -lintl -undefined dynamic_lookup -L/sw/lib  
[parrot] 514 $ ./pdump -h n.pbc
HEADER => [
        wordsize  = 4   (interpreter's wordsize/INTVAL = 4/4)
        byteorder = 1   (interpreter's byteorder       = 1)
        floattype = 0   (interpreter's NUMVAL_SIZE     = 8)
        parrot-version 0.9.0, bytecode-version 3.34
        UUID type = 0, UUID size = 0
        no endianize, no opcode, no numval transform
        dirformat = 1
]

Here's myconfig:

Summary of my parrot 0.9.0 (r36179) configuration:
  configdate='Sat Jan 31 00:58:01 2009 GMT'
  Platform:
    osname=darwin, archname=darwin-2level
    jitcapable=1, jitarchname=ppc-darwin,
    jitosname=DARWIN, jitcpuarch=ppc
    execcapable=1
    perl=/usr/local/bin/perl
  Compiler:
    cc='/usr/bin/gcc', ccflags='-fno-common -no-cpp-precomp  -pipe -I/opt/local/include -pipe -fno-common -Wno-long-double  -DHASATTRIBUTE_CONST  -DHASATTRIBUTE_DEPRECATED  -DHASATTRIBUTE_MALLOC  -DHASATTRIBUTE_NONNULL  -DHASATTRIBUTE_NORETURN  -DHASATTRIBUTE_PURE  -DHASATTRIBUTE_UNUSED  -DHASATTRIBUTE_WARN_UNUSED_RESULT  -falign-functions=16 -fvisibility=hidden -funit-at-a-time -W -Wall -Waggregate-return -Wcast-align -Wcast-qual -Wchar-subscripts -Wcomment -Wdisabled-optimization -Wendif-labels -Wextra -Wformat -Wformat-extra-args -Wformat-nonliteral -Wformat-security -Wformat-y2k -Wimplicit -Wimport -Winit-self -Winline -Winvalid-pch -Wmissing-braces -Wmissing-field-initializers -Wno-missing-format-attribute -Wmissing-include-dirs -Wpacked -Wparentheses -Wpointer-arith -Wreturn-type -Wsequence-point -Wno-shadow -Wsign-compare -Wstrict-aliasing -Wstrict-aliasing=2 -Wswitch -Wswitch-default -Wtrigraphs -Wundef -Wunknown-pragmas -Wno-unused -Wvariadic-macros -Wwrite-strings -Wbad-function-cast -Wdeclaration-after-statement -Wimplicit-function-declaration -Wimplicit-int -Wmain -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wnonnull -I/sw/include -DHAS_GETTEXT',
  Linker and Libraries:
    ld='/usr/bin/g++', ldflags=' -L/opt/local/lib -L/topdir/blib/lib -L/sw/lib',
    cc_ldflags='',
    libs='-lm -lgmp -lreadline -lintl'
  Dynamic Linking:
    share_ext='.dylib', ld_share_flags='-dynamiclib -undefined dynamic_lookup',
    load_ext='.bundle', ld_load_flags='-undefined dynamic_lookup -bundle'
  Types:
    iv=long, intvalsize=4, intsize=4, opcode_t=long, opcode_t_size=4,
    ptrsize=4, ptr_alignment=1 byteorder=4321, 
    nv=double, numvalsize=8, doublesize=8

in reply to: ↑ 5   Changed 5 years ago by jkeenan

Replying to jkeenan:

> [parrot] 514 $ ./pdump -h n.pbc
> HEADER => [
>         wordsize  = 4   (interpreter's wordsize/INTVAL = 4/4)
>         byteorder = 1   (interpreter's byteorder       = 1)
>         floattype = 0   (interpreter's NUMVAL_SIZE     = 8)
>         parrot-version 0.9.0, bytecode-version 3.34
>         UUID type = 0, UUID size = 0
>         no endianize, no opcode, no numval transform
>         dirformat = 1
> ]

By way of contrast, here's the same output from Linux/i386 (where test is passing):

$ ./pdump -h n.pbc
HEADER => [
        wordsize  = 4   (interpreter's wordsize/INTVAL = 4/4)
        byteorder = 0   (interpreter's byteorder       = 0)
        floattype = 0   (interpreter's NUMVAL_SIZE     = 8)
        parrot-version 0.9.0, bytecode-version 3.34
        UUID type = 0, UUID size = 0
        no endianize, no opcode, no numval transform
        dirformat = 1
]

The only differences are in the 'byteorder ... interpreter's byteorder' line.

Thank you very much.
kid51

  Changed 5 years ago by whiteknight

Here's my readout for n.pbc from Ubuntu Intrepid x86_64:

$ parrot 36195> .//pdump -h n.pbc
HEADER => [
	wordsize  = 8	(interpreter's wordsize/INTVAL = 8/8)
	byteorder = 0	(interpreter's byteorder       = 0)
	floattype = 0	(interpreter's NUMVAL_SIZE     = 8)
	parrot-version 0.9.0, bytecode-version 3.34
	UUID type = 0, UUID size = 0
	no endianize, no opcode, no numval transform
	dirformat = 1
]

Here's my readout for i.pbc on the same system:

$ parrot 36195> ./pdump -h i.pbc
HEADER => [
	wordsize  = 8	(interpreter's wordsize/INTVAL = 8/8)
	byteorder = 0	(interpreter's byteorder       = 0)
	floattype = 0	(interpreter's NUMVAL_SIZE     = 8)
	parrot-version 0.9.0, bytecode-version 3.34
	UUID type = 0, UUID size = 0
	no endianize, no opcode, no numval transform
	dirformat = 1
]

I have the pbc files too, where do you want me to send them? Do you want me to attach them here to this ticket?

  Changed 5 years ago by rurban

Attached patch tt254-floattype1-bug.patch  http://nopaste.snit.ch/15469 fixes the pf_item floatval reader if a conversion is requested, and floatval is a normal IEEE-754 8 byte double.

The buf is a wrong advance of 12 byte on (NUMVAL_SIZE == 8 && ! pf->header->floattype)

Changed 5 years ago by rurban

replaces first version from nopaste

follow-ups: ↓ 10 ↓ 13   Changed 5 years ago by jkeenan

rurban, myself and others worked out some of these problems on IRC yesterday, so that the tests are now passing on Darwin/PPC. Could we get some test results from x86_64 or from compilation with ICC?

Thanks.
kid51

in reply to: ↑ 9 ; follow-ups: ↓ 11 ↓ 12   Changed 5 years ago by Infinoid

Replying to jkeenan:

rurban, myself and others worked out some of these problems on IRC yesterday, so that the tests are now passing on Darwin/PPC. Could we get some test results from x86_64 or from compilation with ICC?

Gcc on x86-64 fails:

t/native_pbc/number....1/3 
#   Failed test 'i386 double float 32 bit opcode_t'
#   at t/native_pbc/number.t line 88.
# Exited with error code: [SIGNAL 11]
# Received:
# PackFile_unpack segment 'BYTECODE_t/op/number_1.pasm' directory length 188 length in file 188 needed 186 for unpack
# PackFile_unpack segment 'FIXUP_t/op/number_1.pasm' directory length 0 length in file 0 needed 4 for unpack
# PackFile_unpack segment 'CONSTANT_t/op/number_1.pasm' directory length 1 length in file 1 needed 10610 for unpack
# PackFile_unpack segment 'PIC_idx_t/op/number_1.pasm' directory length 0 length in file 0 needed 4 for unpack
# PackFile_unpack segment 'BYTECODE_t/op/number_1.pasm_DB' directory length 0 length in file 0 needed 4 for unpack
#
# Expected:
# 1
# 4
# 16
# 64
# 256
# 1024
# 4096
# 16384
# 65536
# 262144
# 1048576
# 4194304
# 16777216
# 67108864
# 268435456
# 1073741824
# 4294967296
# 17179869184
# 68719476736
# 274877906944
# 1099511627776
# 4398046511104
# 17592186044416
# 70368744177664
# 281474976710656
# 1.12589990684262e+15
#
# May need to regenerate t/native_pbc/number_1.pbc; see test file

#   Failed test 'PPC double float 32 bit BE opcode_t'
#   at t/native_pbc/number.t line 100.
# Exited with error code: [SIGNAL 3]
# Received:
# PackFile_unpack segment 'BYTECODE_t/op/number_1.pasm' directory length 188 length in file 188 needed 186 for unpack
# PackFile_unpack segment 'FIXUP_t/op/number_1.pasm' directory length 3139056 length in file 3139056 needed 4 for unpack
# Parrot VM: PANIC: Out of mem!
# C file src/gc/memory.c, line 52
# Parrot file (not available), line (not available)
#
# We highly suggest you notify the Parrot team if you have not been working on
# Parrot.  Use parrotbug (located in parrot's root directory) or send an
# e-mail to parrot-porters@perl.org.
# Include the entire text of this error message and the text of the script that
# generated the error.  If you've made any modifications to Parrot, please
# describe them as well.
#
# Version     : 0.9.0-devel
# Configured  : Sun Feb  1 18:28:42 2009 GMT
# Architecture: nojit
# JIT Capable : No
# Interp Flags: (no interpreter)
# Exceptions  : (missing from core)
#
# Dumping Core...
#
# Expected:
# 1
# 4
# 16
# 64
# 256
# 1024
# 4096
# 16384
# 65536
# 262144
# 1048576
# 4194304
# 16777216
# 67108864
# 268435456
# 1073741824
# 4294967296
# 17179869184
# 68719476736
# 274877906944
# 1099511627776
# 4398046511104
# 17592186044416
# 70368744177664
# 281474976710656
# 1.12589990684262e+15
#
# May need to regenerate t/native_pbc/number_2.pbc; see test file
# Looks like you failed 2 tests of 3.
t/native_pbc/number.... Dubious, test returned 2 (wstat 512, 0x200)
 Failed 2/3 subtests 
t/op/number............ok

Test Summary Report
-------------------
t/native_pbc/number (Wstat: 512 Tests: 3 Failed: 2)
  Failed tests:  1-2

Mark

in reply to: ↑ 10   Changed 5 years ago by Infinoid

Replying to Infinoid:

Gcc on x86-64 fails:

That was on r36250. r36251 skips the tests.

in reply to: ↑ 10   Changed 5 years ago by Infinoid

Replying to jkeenan:

rurban, myself and others worked out some of these problems on IRC yesterday, so that the tests are now passing on Darwin/PPC. Could we get some test results from x86_64 or from compilation with ICC?

ICC on x86-64 gets very similar results to my above comments about gcc.

in reply to: ↑ 9   Changed 5 years ago by jkeenan

Replying to jkeenan:

rurban, myself and others worked out some of these problems on IRC yesterday, so that the tests are now passing on Darwin/PPC. Could we get some test results from x86_64 or from compilation with ICC? Thanks.
kid51

Unfortunately, the tests have reverted to failing on both my Darwin and Linux boxes. For Linux, see:  this Smolder report. This was at r36267. So now the situation is worse than it was two days ago.

  Changed 5 years ago by rurban

The remaining errors on 64bit boxes loading 32bit .pbc's are caused by the ALIGN_16 macro in src/packfile.c

Wrong on 64bit platforms:

#define ALIGN_16(st, cursor) \
    (cursor) += (ROUND_16((const unsigned char *)(cursor) - (const unsigned char *)(st))/OPCODE_T_SIZE)

correct macro:

#define ALIGN16(ptr) (const opcode_t *)(((unsigned long)ptr + 15) & (~15))

usage: cursor = ALIGN16(cursor);

I need to test this further.

  Changed 5 years ago by rurban

More 64-bit pbc fixes for TT #254 with r36291. - fix src/packfile.c ALIGN_16() macro for 64-bit - add lots of debugging code, enable with TRACE_PACKFILE 2 - mark more failing tests for 64-bit.

interestingly t/native_pbc/number_3 fails native on gentoo-amd64, but not on solaris-64int.

review: https://trac.parrot.org/parrot/changeset/36291/

There are still two more 64 bit bugs lurking: - ROUND_UP to wordsize on strings - wrong numvals on gentoo-amd64 number_2

  Changed 5 years ago by rurban

  • status changed from assigned to closed
  • resolution set to fixed
  • patch set to applied

Fixed with r36553.

Tests are all enabled, but TODO'd on a DEVEL version.

The 64bit bug will be handled in a different ticket, since it's a cross-platform discrepancy in writer/reader alignment.

  Changed 5 years ago by rurban

  • status changed from closed to reopened
  • platform changed from darwin to all
  • patch changed from applied to new
  • resolution fixed deleted
  • summary changed from t/native_pbc/number.t: New failure on some platforms to pbc compat: t/native_pbc/number.t: New failure on some platforms

found more bugs, my tests pass now.

Fixed passing 64bit tests on 32-bit with tt254-64bit-pbc.tar.gz vice versa to be confirmed.

Changed 5 years ago by rurban

64bit patch + 2 new files

  Changed 5 years ago by rurban

r37020 fixes one of the obvious bugs with the new floatvals for to-be-converted pbc's only. fetch_iv was not initialized properly.

  Changed 5 years ago by rg

I've applied the patch (manually fixing chunks that didn't apply) and I'm sorry to say that neither of the (now enabled) native_pbc tests pass *any* tests on freebsd/amd64. All other tests are still ok. I'll need more time to look at it more closely.

Changed 5 years ago by rg

native pbc _4 and _6 (64bit)

  Changed 5 years ago by rg

I've had some more time to test and so far it looks like if I'm regenerating the pbc files the tests pass. I've attached files for _4 and _6 (amd64 and sparc64) which work on both architectures with a patched parrot. Maybe someone can submit 32bit pbc files to test?

NB: I have no idea why I need to rebuild the pbc files and also if I did it right (a comment says I shouldn't do it with a development build), but that's what works for me. Please let me know what/how else to test ;)

follow-up: ↓ 22   Changed 5 years ago by rurban

Thanks rg.

The pbc's have to be regenerated because PBC_COMPAT was broken, raised up to 3.36, and the bug which didn't warn/fail you about that was just fixed with r37034.

pmc's are indexed by number in the frozen pbc format, so on any major pmc change the pbc's will get incompatible. And there are more pmc deprecations to come soon.

Thanks for the updated pbc's (bc 3.36 I hope), so I can continue testing. I still have no VMM enabled BIOS to able to run 64bit on my laptop.

I've attached my latest minimal patch without pmc_freeze consting.

in reply to: ↑ 21   Changed 5 years ago by rg

Replying to rurban:

Thanks for the updated pbc's (bc 3.36 I hope), so I can continue testing.

Yes, the attached pbcs are bc 3.36. However, I just noticed that we're already at version 3.37, so the tests are failing again. I wonder how we're going to track such a fast moving target (if at all)?

Changed 5 years ago by rurban

  Changed 5 years ago by rurban

  • keywords 64bit added

Summarize: What is missing

- updated t/native_pbc/*.pbc files for every new PBC_COMPAT change. currently 64bit and ppc is misisng.

- 64bit cannot read 32bit pbc's because of wrong ALIGN_16 which is already fixed in this patch, but this patch is too huge for 1.0 and I cannot it right now. I will commit it after 1.0

  Changed 5 years ago by doughera

Today (r37238) t/native_pbc/number.t is failing test 5 for me on OpenSolaris/x86. I have attached the output of the test and my 'myconfig' file.

Changed 5 years ago by doughera

Changed 5 years ago by doughera

  Changed 5 years ago by rurban

tt254-64bit-2.patch fixes now all remaining 64bit bugs when reading 32bit pbc.

1. major bug: ALIGN_16 may not be used on 64bit reading 32bit, or any other low->high combination. We must advance in bytes and not in native ptrs, because we might need the to advance by the half of 8 => 4, which is impossible.

2. minor bug: fetch_op_le_4() reading a signed int (4 byte) to a signed long (8 byte) will not adjust the sign when the long is big enough, but we have negative vtable indices, which we missed. I added a Parrot_Int4 cast since we are expecting 4-byte ints.

There are compiler warnings though which might be fatal on stricter compilers:

src/packfile.c: In function 'pf_register_standard_funcs':
src/packfile.c:1547: warning: initialization from incompatible pointer type
src/packfile.c: In function 'PackFile_Segment_unpack':
src/packfile.c:1869: warning: assignment from incompatible pointer type
src/packfile.c: In function 'directory_unpack':
src/packfile.c:1985: warning: passing argument 2 of 'PF_fetch_opcode' from incompatible pointer type
src/packfile.c:1995: warning: passing argument 2 of 'PF_fetch_opcode' from incompatible pointer type
src/packfile.c:2002: warning: passing argument 2 of 'PF_fetch_cstring' from incompatible pointer type
src/packfile.c:2009: warning: passing argument 2 of 'PF_fetch_opcode' from incompatible pointer type
src/packfile.c:2012: warning: passing argument 2 of 'PF_fetch_opcode' from incompatible pointer type
src/packfile.c:2061: warning: assignment from incompatible pointer type
src/packfile.c:2070: warning: passing argument 2 of 'PF_fetch_opcode' from incompatible pointer type
src/packfile.c:2075: warning: assignment discards qualifiers from pointer target type
src/packfile.c:2110: warning: assignment discards qualifiers from pointer target type

Can someone look at these please?

Changed 5 years ago by rurban

This fixes now the 64bit issues. one major, one minor bug

Changed 5 years ago by NotFound

  Changed 5 years ago by NotFound

Modified to build cleanly with C++: tt254-64bit-3.patch

  Changed 5 years ago by doughera

I haven't had a chance to test this. I picked up the -2.patch version yesterday and tried it overnight, but it didn't build (pf was undefined). It looks like the -3.patch version will probably fix the build problem, but I didn't have a chance to test it. It does look to me like the new line

cursor = (const opcode_t *)((const char *)(self->pf->src) + offs);

will introduce a new 'cast increases required alignment' warning, though I understand the intent is to ensure the alignment is fine. Still the warnings can be annoying. Before proceeding too much further, I think it's important to step back and re-visit the overall design question of how to step through the packfile. Originally, the plan was to go in steps of sizeof(opcode_t). That should work very well for native packfiles, but is proving unworkable for reading foreign packfiles. As the code is currently structured, however, we don't really have foreign and native functions, but individual functions that do the foreign and native switching. Accordingly, I think the higher-level driver code will have to revert to using stepping through the packfile one byte at a time, while paying particular attention to alignment issues in the individual fetching functions. (For one example, see my proposed patch pf_items.p4 in TT #364.) Such reorganization is sufficiently disruptive that it's definitely post-1.0 material.

follow-up: ↓ 29   Changed 5 years ago by rurban

  • status changed from reopened to closed
  • resolution set to fixed

Applied -3 as r37366. https://trac.parrot.org/parrot/changeset/37366/

This strategy still uses nativeptr stepping (fast) and not the more general stepping in bytes (slow but easier to understand).

It might introduce a new 'cast increases required alignment' warning on cc, but not on gcc and msvc, and it is safe to use, since it points the cursor to a properly aligned address.

2. Changing to stepping in bytes should get thorough testing, but would increase readability a lot. Maybe we find a way to get rid of your cast increases required alignment warning.

I also would like to benchmark that change, esp. on big pbc's such as perl6.pbc

in reply to: ↑ 28   Changed 5 years ago by doughera

2. Changing to stepping in bytes should get thorough testing, but would increase readability a lot. Maybe we find a way to get rid of your cast increases required alignment warning.

The way to avoid the warning (which will show up with gcc on SPARC) is to advance cursor in opcode_t units. That is, advance by offs/sizeof(opcode_t) steps. Since pf->src is already an (opcode_t *), and you've padded offs out to a multiple of 16, it should work cleanly without warning.

follow-up: ↓ 31   Changed 5 years ago by doughera

  • status changed from closed to reopened
  • resolution fixed deleted

I have attached a patch to get rid of the new warning. I debated opening a different ticket, since this has now strayed from the original issue, but decided this was a fairly obvious final follow-up, and should stay here. More substantial changes should get a new ticket.

Changed 5 years ago by doughera

in reply to: ↑ 30 ; follow-up: ↓ 34   Changed 5 years ago by jkeenan

Replying to doughera:

I have attached a patch to get rid of the new warning. I debated opening a different ticket, since this has now strayed from the original issue, but decided this was a fairly obvious final follow-up, and should stay here. More substantial changes should get a new ticket.

I'd like to get this ticket resolved.

What I plan to do is, after next week's release, to test Andy D's last patch out on the two OSes to which I have access (Linux/i386 and Darwin/PPC) and, if it does no harm and if there are no objections, to apply it and close the ticket. After that point, any problems with t/native_pbc/number.t should be discussed in a new TT.

Please speak up if you don't agree with that plan.

Thank you very much.
kid51

Changed 5 years ago by jkeenan

Andy D's patch adjusted for c_parens coding standard

  Changed 5 years ago by jkeenan

  • status changed from reopened to new
  • owner changed from rurban to jkeenan

  Changed 5 years ago by jkeenan

  • status changed from new to assigned

in reply to: ↑ 31 ; follow-up: ↓ 35   Changed 5 years ago by jkeenan

  • patch changed from new to applied

Replying to jkeenan:

I'd like to get this ticket resolved. What I plan to do is, after next week's release, to test Andy D's last patch out on the two OSes to which I have access (Linux/i386 and Darwin/PPC) and, if it does no harm and if there are no objections, to apply it and close the ticket. After that point, any problems with t/native_pbc/number.t should be discussed in a new TT.

Patch applied in r38264. Ticket will be closed within 2 days unless there is serious objection.

Thank you very much.
kid51

in reply to: ↑ 34   Changed 5 years ago by jkeenan

  • status changed from assigned to closed
  • resolution set to fixed

Replying to jkeenan:

Patch applied in r38264. Ticket will be closed within 2 days unless there is serious objection.

Two days have passed without objection, so I'm closing this ticket.

Thank you very much.
kid51

Note: See TracTickets for help on using tickets.