jerryscript-project / jerryscript

Ultra-lightweight JavaScript engine for the Internet of Things.
https://jerryscript.net
Apache License 2.0
6.89k stars 669 forks source link

String.startsWith fails with -Ofast #5042

Open ReimuNotMoe opened 1 year ago

ReimuNotMoe commented 1 year ago

When JerryScript is compiled with -Ofast, String.startsWith won't work correctly anymore.

This problem doesn't happen with lower optimization levels.

I noticed some functions in ecma-builtin-helpers.c are using ecma_number_t in for loops. Is this really necessary? Since ecma_number_t is defined as double, this can cause some problems. I'm not sure if it's the case but I suspect it's related.

JerryScript revision

1a2c04763aba49f52b1537acd3730098c873511c

Build platform

uname -a

Linux (none) 5.19.14-x1000 #477 PREEMPT Wed Dec 7 04:29:41 CST 2022 mips GNU/Linux

/proc/cpuinfo

system type             : X1000E
machine                 : SudoMaker X1000 Nano EVB
processor               : 0
cpu model               : Ingenic XBurst V4.15  FPU V0.0
BogoMIPS                : 1197.05
wait instruction        : yes
microsecond timers      : no
tlb_entries             : 32
extra interrupt vector  : yes
hardware watchpoint     : yes, count: 1, address/irw mask: [0x0fff]
isa                     : mips1 mips2 mips32r1 mips32r2
ASEs implemented        :
Options implemented     : tlb 4kex 4k_cache fpu 32fpr prefetch mcheck ejtag llsc vtag_icache perf_cntr_intr_bit nan_legacy
shadow register sets    : 1
kscratch registers      : 0
package                 : 0
core                    : 0
VCED exceptions         : not available
VCEI exceptions         : not available

gcc -v

Using built-in specs.
COLLECT_GCC=/data2/buildroot-2022.02.2-cle/output/host/bin/mipsel-buildroot-linux-gnu-gcc.br_real
COLLECT_LTO_WRAPPER=/data2/buildroot-2022.02.2-cle/output/host/libexec/gcc/mipsel-buildroot-linux-gnu/11.3.0/lto-wrapper
Target: mipsel-buildroot-linux-gnu
Configured with: ./configure --prefix=/data2/buildroot-2022.02.2-cle/output/host --sysconfdir=/data2/buildroot-2022.02.2-cle/output/host/etc --enable-static --target=mipsel-buildroot-linux-gnu --with-sysroot=/data2/buildroot-2022.02.2-cle/output/host/mipsel-buildroot-linux-gnu/sysroot --enable-__cxa_atexit --with-gnu-ld --disable-libssp --disable-multilib --disable-decimal-float --with-gmp=/data2/buildroot-2022.02.2-cle/output/host --with-mpc=/data2/buildroot-2022.02.2-cle/output/host --with-mpfr=/data2/buildroot-2022.02.2-cle/output/host --with-pkgversion='Buildroot 2022.02.2' --with-bugurl=http://bugs.buildroot.net/ --without-zstd --disable-libquadmath --disable-libquadmath-support --enable-tls --enable-threads --without-isl --without-cloog --with-arch=mips32r2 --with-abi=32 --with-nan=legacy --with-fp-32=xx --enable-languages=c,c++ --with-build-time-tools=/data2/buildroot-2022.02.2-cle/output/host/mipsel-buildroot-linux-gnu/bin --enable-shared --disable-libgomp
Thread model: posix
Supported LTO compression algorithms: zlib
gcc version 11.3.0 (Buildroot 2022.02.2)

Build steps

set(CFLAGS_COMMON -march=mips32r2 -mno-interlink-compressed -mno-shared -Ofast --param=l1-cache-size=16 --param=l1-cache-line-size=32 --param=l2-cache-size=128)
add_compile_options(${CFLAGS_COMMON})
add_link_options(${CFLAGS_COMMON} -Wl,--gc-sections)

set(JERRY_ERROR_MESSAGES ON CACHE BOOL "" FORCE)
set(JERRY_LINE_INFO ON CACHE BOOL "" FORCE)
set(JERRY_MEM_STATS ON CACHE BOOL "" FORCE)
set(JERRY_CMDLINE OFF CACHE BOOL "" FORCE)
set(JERRY_EXTERNAL_CONTEXT OFF CACHE BOOL "" FORCE)

Test case

s = "LV_PART_MAIN";
s.startsWith("LV_PART_");

Output

false

Expected behavior

true

nekopsykose commented 1 year ago

-Ofast

you built using options documented to break the standard, and you're surprised something broke?

ReimuNotMoe commented 1 year ago

you built using options documented to break the standard, and you're surprised something broke?

I'm not the only one with fault here since many incorrect coding practises can be discovered with high optimization levels. So it's a problem worth discussing about.

zherczeg commented 1 year ago

Isn't mips officially abandoned? Maybe unmaintained compiler tools cause issues.

nekopsykose commented 1 year ago

So it's a problem worth discussing about.

sure- but only insofar as something being wrong because it is wrong based on some level of correctness, not related to Ofast itself.

Isn't mips officially abandoned? Maybe unmaintained compiler tools cause issues.

maybe, maybe not, but for a simple test, i couldn't reproduce this on x86_64-linux-musl with either gcc12.2 or clang15 with -Ofast.

(that, of course, doesn't rule out a correctness issue- UB code does not always fail all the time on every platform, but it's possible everything is correct and this is purely a bad compiler/platform issue, as noted above)

ReimuNotMoe commented 1 year ago

@zherczeg

Isn't mips officially abandoned? Maybe unmaintained compiler tools cause issues.

No it's not. Waves sold all MIPS IPs to CIP United and the development is being continued. Nowadays the latest 5G basebands in Mediatek chipsets still uses MIPS architecture.

The gcc version is 11.3.0, which should be up-to-date enough.

What's your opinion about using double in for loops? Do you think they're necessary or should be replaced with integers?

zherczeg commented 1 year ago

Do you have a link for that? It probably depends on the case.

ReimuNotMoe commented 1 year ago

Do you have a link for that? It probably depends on the case.

One example: ecma-builtin-helpers.c#L492

And there are more.

zherczeg commented 1 year ago

This one can be changed. Though it should not cause any issues.

ClassicOldSong commented 1 year ago

indexOf has the same problem, most likely they use the same mechanism for indexing