Internal compiler error triggered by preprocessor inspecting code comments

[sgbeal]

Please include the following in your bug report:

Version of emscripten/emsdk:

emcc (Emscripten gcc/clang-like replacement + linker emulating GNU ld) 3.1.31 (e88336121cfe6da4a96c88e46f314552f07dfed0)

The build itself is extremely "involved", but the reason for the breakage is clear...

The message:

Internal compiler error in src/compiler.js!
Please create a bug report at https://github.com/emscripten-core/emscripten/issues/
with a log of the build and the input files used to run. Exception message: "Error: /home/stephan/f/s/lite/ext/wasm/bld/post-js.vanilla.js:8566: Unknown preprocessor directive #1",
    at preprocess (/home/stephan/src/emsdk/upstream/emscripten/src/parseTools.js:131:19)
    at includeFile (/home/stephan/src/emsdk/upstream/emscripten/src/jsifier.js:458:25)
    at finalCombiner (/home/stephan/src/emsdk/upstream/emscripten/src/jsifier.js:530:7)
    at runJSify (/home/stephan/src/emsdk/upstream/emscripten/src/jsifier.js:554:3)
    at Object.<anonymous> (/home/stephan/src/emsdk/upstream/emscripten/src/compiler.js:97:3)
    at Module._compile (internal/modules/cjs/loader.js:1085:14)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1114:10)
    at Module.load (internal/modules/cjs/loader.js:950:32)
    at Function.Module._load (internal/modules/cjs/loader.js:790:12)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:76:12)

The cause of that is a comment block with "#1" in it. About halfway down...

/*
  2022-07-22

  The author disclaims copyright to this source code.  In place of a
  legal notice, here is a blessing:

  *   May you do good and not evil.
  *   May you find forgiveness for yourself and forgive others.
  *   May you share freely, never taking more than you give.

  ***********************************************************************

  This file implements the initializer for the sqlite3 "Worker API
  #1", a very basic DB access API intended to be scripted from a main
  window thread via Worker-style messages. Because of limitations in
  that type of communication, this API is minimalistic and only
  capable of serving relatively basic DB requests (e.g. it cannot
  process nested query loops concurrently).

  This file requires that the core C-style sqlite3 API and OO API #1
  have been loaded.
*/

Reformatting the comment so that the line does not start with the hash sign works around it, but doing so requires rephrasing the documentation so that the auto-wrapper just happens to not place that token at the start of the line. Any given future edits and auto-wrapping might undo that kludge.

[sbc100]

Yes, our pre-processor is a little basic I'm afraid. Sadly it looks like handling comments like this correctly might add quite a bit a complexity. If you want to take a look at the code it's here:

emscripten/src/parseTools.js

Lines 34 to 37 in b08191b

	// Simple #if/else/endif preprocessing for a file. Checks if the
	// ident checked is true in our global.
	// Also handles #include x.js (similar to C #include <file>)
	function preprocess(filename) {

[sgbeal]

Indeed, handling comments correctly is a pain in the butt, but generating a compilation error for perfectly legitimate client code which doesn't have any reliance on that preprocessor is "just wrong." Rather than add proper comment support, perhaps the preprocessor should be opt-in? Certainly only a minority of client code makes use of it?

[sbc100]

What do you think would be good way to opt in? I'd rather avoid an entirely new --pre-js-preprocessed flag.

I'm think maybe we can just remove that error, and ignore unknown directives.

[sgbeal]

i was initially (and perhaps naively) thinking --preprocess would enable it globally, but making it opt-in may well break any number of client trees, so i'm no longer a fan of that approach. Silently skipping unknown directives sounds destined to cause more grief than incorrectly reading directives from comments. i'd currently rather see it stay as-is than take either of those approaches, on the grounds that the current code seems likely to break with the fewest clients and has a relatively painless (just annoying) client-side workaround.

Perhaps... rather than extending the preprocessor to support comments, add a comments-stripping-only preprocessor step before the current preprocessor. That would keep the comment-stripping complexity out of the core preprocessor. The only(?) tricky part about stripping comments is making sure not to strip them from inside string literals. As a real-world case of that, in the sqlite project we have some SQL strings which contain /*...*/ comments.

[sbc100]

Sadly its not just you can has run into this issue We got another report recently from someone using ES6 private members: #18724. In this case there is no good workaround for the user, so I think the status quo cannot stand.

The way I see it that leave 2 choices:

1. Make a smarter, context aware parser that can handle both comments a private member fields.
2. Ignore unknown directives
3. Make preprocessing optional, on a per-file bases (perhaps in combination with 2).

With the added complexity private members I don't think (1) is feasible.

The risks of (2) are that there are some rare cases were user might use #if or #else or #endif as private members or in comments. This seems exceeding unlikely, but also is pretty gross failure mode. Is this what you mean when you say "destined to cause grief", or did you have something else in mind?

So I think (3) is our best option. One questions is, do we make this opt-on or opt-out. It seems like if we ignore unknown directives, then making it opt-out could work, particularly if we could detect failure cases and recommend the opt-out? What about something like "Encountered unknown preprocessor directive. If this is not actually a directive for the pre-processor you can opt by doing "?

[sgbeal]

This seems exceeding unlikely, but also is pretty gross failure mode. Is this what you mean when you say "destined to cause grief", or did you have something else in mind?

That was precisely the kind of thing i had in mind: completely innocent docs/comments which cause it to choke.

Private members are something i had not considered at all. That's a whole other can of worms.

[sgbeal]

What if the preprocessor syntax was changed? e.g. in the sqlite project we preprocess some of our JS files but the CPP preprocessor just makes a mess of it, so we use use a custom preprocessor with a configurable directive marker. We use:

//#if ...
...
//#else
...
//#endif

they look like comments to JS but //# is the delimiter for the preprocessor. Perhaps using ## as a delimiter would suffice for emscripten?

[sbc100]

Changing the pre-process syntax would be tricky since it would effect all the third-party JS library code that relies on it.

Have a different pre-processor for JS library code and for pre/post-js also seems like a bad outcome.

I'm still leaning towards the "explicit opt-out along with useful error/hint when opt out is required/needed"

[sgbeal]

I'm still leaning towards the "explicit opt-out along with useful error/hint when opt out is required/needed"

FWIW, that sounds like the least painful option all around.

[hostilefork]

I have an error which looks suspiciously related. It started happening in SDK 3.1.31 (no problems in 3.1.30)

However it's an assertion failure:

Internal compiler error in src/compiler.js!
Please create a bug report at https://github.com/emscripten-core/emscripten/issues/
with a log of the build and the input files used to run. Exception message: "AssertionError [ERR_ASSERTION]: false == true
    at preprocess (/home/ae1020/emsdk/upstream/emscripten/src/parseTools.js:120:13)
    at includeFile (/home/ae1020/emsdk/upstream/emscripten/src/jsifier.js:458:25)
    at finalCombiner (/home/ae1020/emsdk/upstream/emscripten/src/jsifier.js:530:7)
    at runJSify (/home/ae1020/emsdk/upstream/emscripten/src/jsifier.js:554:3)
    at Object.<anonymous> (/home/ae1020/emsdk/upstream/emscripten/src/compiler.js:97:3)
    at Module._compile (internal/modules/cjs/loader.js:1085:14)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1114:10)
    at Module.load (internal/modules/cjs/loader.js:950:32)
    at Function.Module._load (internal/modules/cjs/loader.js:790:12)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:76:12)

My case is also extremely..."involved". 🙄 So if it's conceivable that this assert would trigger given knowledge already present I can use 3.1.30 until it is fixed.