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

Incorrect result of mujs #152

Closed
shao-hua-li opened this issue Oct 27, 2021 · 7 comments
Closed

Incorrect result of mujs #152

shao-hua-li opened this issue Oct 27, 2021 · 7 comments

Comments

@shao-hua-li
Copy link

Hi there,

  • Version: commit daa2241 (git head)
  • Compiler: gcc11
  • Compile args: CC=gcc CXX=g++ make

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
Copy link
Owner

ccxvii commented Nov 1, 2021

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
Copy link
Author

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 #153 for this workaround.

@avih
Copy link
Contributor

avih commented Nov 2, 2021

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
Copy link
Owner

ccxvii commented Nov 3, 2021

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
Copy link
Author

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
Copy link
Owner

ccxvii commented Nov 3, 2021

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
Copy link
Owner

ccxvii commented Dec 6, 2021

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

@ccxvii ccxvii closed this as completed Dec 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants