search

ccxvii / mujs

An embeddable Javascript interpreter in C.
http://mujs.com/
ISC License
813 stars 98 forks source link

Incorrect result of mujs #152

Closed shao-hua-li closed 2 years ago

shao-hua-li commented 3 years ago

Hi there,

mujs has a checkfutureword() function in jscompile.c that is designed to limit a few keywords like "class", "const", "enum", etc. However, I found this function would not be functioning if I the words are in function parameters. For example, for the following illegal fragment, mujs would compile it normally :

(function(const){})

I tried to debug the code. I found that function body of static void cparams(JF, js_Ast *list, js_Ast *fname) in jscompile.c:1322 has been optimised out by gcc11 -O1 and above, which makes the check in cfunbody() always invalid. If I use the following compile args: CC=gcc CXX=g++ XCFLAGS="-O0" make, then mujs would raise error as expected.

ccxvii commented 3 years ago

This is making me very confused. My initial guess is that something somewhere is tripping up and letting GCC run wild with undefined behavior optimizations. Over-eager optimizing compiler writers are the usual villains in this kind of story...

shao-hua-li commented 3 years ago

Yes, I agree. A less nice work around for this issue is to ask gcc not to optimise the checkfutureword() function, i.e., add attribute((optimize("O0"))) to the function. I've created a pull request https://github.com/ccxvii/mujs/pull/153 for this workaround.

avih commented 3 years ago

ask gcc not to optimise the checkfutureword()

That's not a good idea, because you don't actually know what causes it to mess this specific function. There could be other functions with the same issue which we don't know about, and this will not fix them.

The solution should be either to disable optimizations completely, or understand exactly why this function is messed up, and then find a solution which fixes it for all functions which have the same issue.

ccxvii commented 3 years ago

This turns out to be a bug in GCC's optimizer. See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103052 for more details.

shao-hua-li commented 3 years ago

This turns out to be a bug in GCC's optimizer. See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103052 for more details.

This is so cool that you analysed it out! I tried but failed to reduce the mujs code to a simpler test case. It's really nice to see that you made it. May ask you how you did that? Is there any tool you used?

ccxvii commented 3 years ago

No tools, just a lot of manual labor :) It took a couple of hours. I started by making a test program that only called cparams() by manually constructing the AST nodes. Once that program could replicate the bug, I deleted all the other mujs functions that were not used. After simplifying those that remained as much as I could while still triggering the bug, and "inlining" chains of function calls I ended up with the final test case.

ccxvii commented 2 years ago

There's a workaround that we need to keep in place until newer versions of GCC with the bug fix are common enough.