Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Runtime errors on fa526 (arm4tl): res != DUK_TVAL_GET_NUMBER(tv) (duk_hobject_props.c:2628) #127

Closed
jbysewski opened this issue Mar 6, 2015 · 22 comments
Milestone

Comments

@jbysewski
Copy link

I was able to compile duktape to run on the fa526 (arm4tl) processor in a MOXA UC7101 device.
With debug logging and assertions enabled I see, that it is able to create the heap and run an gc.
To rule out my own application which runs fine under generic linux x86 and OS X hosts, I reproduced the error with the cmdline example: duk.

It boils down to:

PANIC 54: assertion failed: (duk_double_t) res == DUK_TVAL_GET_NUMBER(tv) (duk_hobject_props.c:2628) (calling abort)

Compiler defines:
http://pastebin.com/H7dKiLrM

duk execution log:
http://pastebin.com/rV7i9cYQ

@svaarala svaarala added the bug label Mar 6, 2015
@svaarala
Copy link
Owner

svaarala commented Mar 6, 2015

If you didn't already have it defined, could you check what happens with -DDUK_OPT_SELF_TESTS enabled?

@svaarala
Copy link
Owner

svaarala commented Mar 6, 2015

Another thing you could try (to help in diagnosis) is -DDUK_OPT_NO_PACKED_TVAL.

@jbysewski
Copy link
Author

It seems the execution log is no different:
http://pastebin.com/hsNtZkPy

Compiled with:

-DDUK_OPT_DEBUG
-DDUK_OPT_DPRINT
-DDUK_OPT_ASSERTIONS
-DDUK_OPT_SELF_TESTS
-DDUK_OPT_NO_PACKED_TVAL
-DDUK_CMDLINE_ALLOC_LOGGING

Allocation log:
http://pastebin.com/MhZ0tZeW

Execution log without assertions:
http://pastebin.com/7L9p4KJv

duk_js_compiler.c:434 (duk__advance_helper): parse error: expect=52, got=55
Expect 52 = DUK_TOK_RCURLY, got 55 = DUK_TOK_LPAREN?

@jbysewski
Copy link
Author

The target has an ancient uClibc (9.26), here is the uClibc_config.h
http://pastebin.com/TGtM5FSn

Unfortunately I can't upgrade the uClibc easily.
Maybe it is a problem with softfloat/hardfloat?

@svaarala
Copy link
Owner

svaarala commented Mar 6, 2015

I'd also suspect the issue is related to IEEE double handling - but the known cases are caught by the self tests (e.g. clang/llvm union aliasing issue with -m32).

It might also be that Duktape ends up determining endianness incorrectly. On that front:

  • Are you compiling with -std=c99? If not, that's worth a try.
  • If the target is mixed endian (IEEE doubles), you could try -DDUK_OPT_FORCE_BYTEORDER=2 (1 for little endian)
  • Unlikely, but could try -DDUK_OPT_FORCE_ALIGN=8

@svaarala
Copy link
Owner

svaarala commented Mar 7, 2015

The define dump has:

#define __ARCH_LITTLE_ENDIAN__ 1
#define __FLOAT_WORD_ORDER__ __ORDER_BIG_ENDIAN__
#define __BYTE_ORDER__ __ORDER_LITTLE_ENDIAN__

With these Duktape should end up thinking the platform is mixed endian (little endian 64-bit integers but mixed endian IEEE doubles), i.e. same as -DDUK_OPT_FORCE_BYTEORDER=2.

Just to make sure, could you compile https://github.com/svaarala/duktape/blob/master/util/check_align.c and run the following:

./check_align uint64_t 0
./check_align double 0

@jbysewski
Copy link
Author

check_align:

~ # ./check_align uint64_t 0
uint64_t offset 0, ptr 0xf17d00
88 77 66 55 44 33 22 11 = 0x1122334455667788
little endian
~ # ./check_align double 0
double offset 0, ptr 0xf17d00
bb eb 78 43 e1 d0 ee 95 = 112233445566778890.000000
mixed endian

-std=c99 or even -std=gnu99 was used.
-DDUK_OPT_FORCE_ALIGN=8 made no difference.

The compiler I use is a selfbuild arm-elf-gcc (GCC) 4.6.3.
With some fiddling (e.g. no __builtin_inf) I was able to compile duk with the original
compiler from the hardware vendor, which is an even more ancient gcc 2.95.3 (released on March 16, 2001!), this executable works as expected. sigh
Unfortunately we need c99 support and some other features and therefore cannot use this compiler for our application. check_align gives the same results when compiled with this compiler with only the pointer address being different.

The builtin defines are quite short, maybe that's a helpful clue?

# ~/moxa/arm-elf-gcc -dM -E - < /dev/null
#define __arm_elf 1
#define arm_elf 1
#define __arm__ 1
#define __arm 1
#define __arm_elf__ 1
#define __ARM_ARCH_4T__ 1
#define __GNUC_MINOR__ 95
#define arm 1
#define __CHAR_UNSIGNED__ 1
#define __APCS_32__ 1
#define __GNUC__ 2
#define __ELF__ 1

@svaarala
Copy link
Owner

svaarala commented Mar 7, 2015

Quite an interesting situation, but I'm sure we can figure it out :-)

Did you try the -DDUK_OPT_FORCE_ALIGN option -- just to rule out that Duktape ends up detecting endianness incorrectly?

Also if you could dump the very verbose debug logs (-DDUK_OPT_DPRINT, -DDUK_OPT_DDPRINT, and -DDUK_OPT_DDDPRINT) that might give a clue as to what goes wrong.

@svaarala svaarala modified the milestones: v1.1.2, v1.2.0 Mar 7, 2015
@jbysewski
Copy link
Author

-DDUK_OPT_FORCE_ALIGN=8 was configured.

Here is the really verbose 7.2MB log:
https://gist.githubusercontent.com/jbysewski/b4f2ca22ae33ff4d4e9f/raw/42f5ce5dcfa11e656a801d53d701183beadc5346/duk_assertion_fail_verbose.txt

@jbysewski
Copy link
Author

Here is the execution log from the ancient but working gcc-2.95.3 compiler version:
https://gist.github.com/jbysewski/0b91e232deda65e3c2cc/raw/4579cc9dbbd8ef50c6a56c60f7efc0ddb95bc5f2/duk_original_compiler.txt

They don't match very well after the self-tests, the good log is only 4MB large.
One thing that caught my attention was line 144:

good: [DDD] duk_heap_stringtable.c:651 (duk__resize_strtab_raw_probe): attempt to resize stringtable: 23 entries, 92 bytes, 1 used, 4% load -> 23 entries, 92 bytes, 1 used, 4% load
 bad: [DDD] duk_heap_stringtable.c:651 (duk__resize_strtab_raw_probe): attempt to resize stringtable: 23 entries, 92 bytes, 1 used, 4% load -> 23 entries, 92 bytes, 1 used, 2147483647% load

The good one shows 4% load, the bad one shows 2147483647% load.

@jbysewski
Copy link
Author

Also the bad one has lots of occurences of dragon4 (from duk_numconv.c) while the good one has not one.

@svaarala
Copy link
Owner

svaarala commented Mar 9, 2015

That load debug is printed by:

        DUK_DDD(DUK_DDDPRINT("attempt to resize stringtable: %ld entries, %ld bytes, %ld used, %ld%% load -> %ld entries, %ld bytes, %ld used, %ld%% load",
                             (long) old_size, (long) (sizeof(duk_hstring *) * old_size), (long) old_used,
                             (long) (((double) old_used) / ((double) old_size) * 100.0),
                             (long) new_size, (long) (sizeof(duk_hstring *) * new_size), (long) duk__count_used_probe(heap),
                             (long) (((double) duk__count_used_probe(heap)) / ((double) new_size) * 100.0)));

There are other %ld formats that work correctly there so it's probably not formatting. Unfortunately it seems it might either double arithmetic or double-to-integer casts. I've encountered compiler setups with both issues so not sure which is more probable.

@svaarala
Copy link
Owner

svaarala commented Mar 9, 2015

The Dragon4 difference is probable due to this double-to-string fast path check:

        uval = (unsigned int) x;
        if (((double) uval) == x &&  /* integer number in range */
            flags == 0) {            /* no special formatting */

I'm guessing the same cast issues cause this fast path to missed in the "bad" case, which results in Duktape going through the much more complicated (and verbose) slow path.

@jbysewski
Copy link
Author

I think I've found the underlying problem: the unsigned int to float conversion is broken in my compiler. The internal libgcc function __floatunsidf does not work as expected.

Here's a little test program:

#include <stdio.h>

double __floatunsidf (unsigned i)
{
  double r = (double)(int)i;
  if ((int)i < 0)
    r += 0x1p32f;
  return r;
}

int main()
{
  double  d = 1.0;
  unsigned int u = (unsigned int) d;
  double d2 = (double) u;

  printf("d: %f d2: %f u: %d equal: %d\n", d, d2, u, ((double)u) == d);
  return 0;
}

With the custom __floatunsidfit works as it should and outputs:

d: 1.000000 d2: 1.000000 u: 1 equal: 1

Without it the output is:

d: 1.000000 d2: 0.000000 u: 1 equal: 0

The failure might be associated with this gcc-bug, where I also got the __floatunsidf implementation from:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=24998

Thank you very much for your support!

@svaarala
Copy link
Owner

svaarala commented Mar 9, 2015

Great :) Did you figure out how to fix it?

I'll add cast tests to the self test set, because there are now at least 3 cases where they have been broken. It's often fixable but it's hard to pin down so it'd be nice to self test for.

@jbysewski
Copy link
Author

For our application I see the following options:

  • compile libduktape with the ancient non-c99 compiler, compile our app with the newer one (works)
  • use the __floatunsidf from above (kind of a hack, but seems to work)
  • try another compiler version (might work, but the uClibc is compiled with old linux ABI so nothing newer than 4.6 works)
  • go through all the hassle of upgrading the kernel and libc for these devices sigh
  • see if we can get away with not supporting this hardware (not likely as it's an reasonably cheap embedded linux device with industrial temperature range)

An self test is easy enough and seems like a good idea because this took lots of time to pin down, like you said.

@svaarala
Copy link
Owner

svaarala commented Mar 9, 2015

Another approach: Duktape does a double-to-integer and integer-to-double cast in a reasonable amount of places so it'd also be possible to make those casts explicit through macros. This would allow the casts used by Duktape to be reimplemented to avoid problems in the compiler environment.

Another dependency Duktape has is a sprintf() with working %d, %ld, %p at least. This is also needed in only a handful of places so it'd be possible to make it retargetable.

@jbysewski
Copy link
Author

You've gone a long way with supporting a lot of different host/target configurations.
So yes, that seems like a nice solution, too.

@svaarala
Copy link
Owner

svaarala commented Mar 9, 2015

One counterargument to fixing this by avoiding such casts in Duktape is that other code (such as existing Duktape/C binding code) might still break due to problematic casts.

But if one is writing all the C code from scratch and can deal with compiler deficiencies it certainly is feasible. I've had to work with closed toolchains with similar issues, e.g. one has working double-to-integer casts up to 32-bit range but beyond 32 bits the cast results start to go wrong. In that particular case it was possible to just avoid the offending casts.

Anyway, it'd be interesting to hear if you can resolve this on the compiler end somehow, because people might encounter similar problems.

@svaarala svaarala added portability and removed bug labels Mar 20, 2015
svaarala added a commit that referenced this issue Mar 25, 2015
@svaarala
Copy link
Owner

@jbysewski Any updates on this issue re: compiler configuration? :)

@jbysewski
Copy link
Author

It seems the compiler is a bit weird, but I had no time rebuilding the compiler suite and playing with the different soft/hard-float options gcc has available. Currently we're running with the __floatunsidf-hack which works without any further hassle.
Good to see you added a self test for this kind of misbehaviour as that might save the next poor guy some time. :)

@svaarala
Copy link
Owner

svaarala commented Apr 2, 2015

Good to hear - I'll close this issue because there's no clear Duktape change (except the added self test).

@svaarala svaarala closed this as completed Apr 2, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants