jerryscript icon indicating copy to clipboard operation
jerryscript copied to clipboard

String.startsWith fails with -Ofast

Open ReimuNotMoe opened this issue 2 years ago • 9 comments

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

ReimuNotMoe avatar Feb 11 '23 20:02 ReimuNotMoe

-Ofast

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

nekopsykose avatar Feb 11 '23 20:02 nekopsykose

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.

ReimuNotMoe avatar Feb 11 '23 20:02 ReimuNotMoe

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

zherczeg avatar Feb 12 '23 08:02 zherczeg

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)

nekopsykose avatar Feb 12 '23 10:02 nekopsykose

@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?

ReimuNotMoe avatar Feb 12 '23 11:02 ReimuNotMoe

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

zherczeg avatar Feb 12 '23 15:02 zherczeg

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

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

And there are more.

ReimuNotMoe avatar Feb 12 '23 17:02 ReimuNotMoe

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

zherczeg avatar Feb 12 '23 18:02 zherczeg

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

ClassicOldSong avatar Feb 12 '23 19:02 ClassicOldSong