Compilation Error in quickjs.c without -DCONFIG_BIGNUM Option

[lanytcc]

Description: When compiling QuickJS without the -DCONFIG_BIGNUM option, I encountered a compilation error related to the handling of BC_TAG_BIG_DECIMAL in quickjs.c. The error occurs because the conditional compilation directive #ifdef CONFIG_BIGNUM is missing around certain code blocks that should only be included when Bignum support is enabled. This issue is present at line numbers 35952 and similarly at line 35960.

Steps to Reproduce:

1. Compile QuickJS without the -DCONFIG_BIGNUM option.
2. Observe the compilation errors at lines 35952 and 35960 in quickjs.c.

Expected Behavior: The code should compile without errors when the -DCONFIG_BIGNUM option is not used. Conditional compilation directives should be correctly applied to exclude Bignum-related code blocks.

Actual Behavior: Compilation errors occur due to the presence of Bignum-related code without the -DCONFIG_BIGNUM compilation flag.

Suggested Fix: Wrap the Bignum-related code sections with #ifdef CONFIG_BIGNUM ... #endif to ensure they are only compiled when Bignum support is enabled. For example:

#ifdef CONFIG_BIGNUM
if (tag != BC_TAG_BIG_DECIMAL)
    l = (len + sizeof(limb_t) - 1) / sizeof(limb_t);
else
#endif

Additional Context: This issue affects the build process in environments where Bignum support is not required or desired, potentially hindering the use of QuickJS in those environments.

[chqrlie]

@lanytcc: I shall fix this problem.

The macro symbol USE_BIGNUM is somewhat misleading, it used to disable support for both BigFloat, BigDecimal and BitInt types. We should rename it to USE_BIGFLOAT.

Do you have a target where you would want to disable BitInt too?

BigInt support is not optimal at the moment because it uses the same representation as BigFloat. We should improve this in the coming months.

[past-due]

Do you have a target where you would want to disable BitInt too?

We currently patch things ourselves to disable BigInt support for some targets, so if you can reintroduce a define that does so that would be appreciated by us at least.

[saghul]

Why do you need to disable it though?

[past-due]

Why do you need to disable it though?

We have scenarios in which we create contexts with a limited subset of JS language support, and we disable features such as BigInt, atomics, etc that are unneeded in those scenarios to reduce the surface area.

I'm not necessarily saying this is a common use-case, but since the question was raised, I figured I'd mention that we do in fact sometimes want to compile without functionality we don't need (and BigInt support is one example we previously omitted with the earlier define) - especially for embedding situations.

[saghul]

Right.

You could still do that by creating a raw context and not adding big int support, even though support for it could be builtin.

That's the path we chose for our friendly fork.

We'll likely follow the same path with atomics, once support for Windows is added...

[lanytcc]

@chqrlie thanks a lot for getting back to me so quickly and letting me know you're on it. It's cool that you're thinking about renaming the macro to clear things up. I personally don't need to turn off BigInt right now, but having the option to tweak things more precisely sounds like it could help others out. Appreciate your effort and the work you're putting into making the project better for everyone.

[lanytcc]

@saghul thanks for asking! I typically use QuickJS in conjunction with dynamic C libraries to strike a balance between performance and flexibility, with JavaScript acting mainly as a glue language in my projects. For this reason, the Bignum feature doesn’t really serve a purpose for me. Plus, I’m not entirely sure if enabling Bignum could impact the performance of operations on simple numbers. So, as a programmer, my first instinct is to turn off features that aren't necessary for my specific use case. Hope that clarifies things!

[saghul]

I’m not entirely sure if enabling Bignum could impact the performance of operations on simple numbers.

It doesn't, AFAIK.

So, as a programmer, my first instinct is to turn off features that aren't necessary for my specific use case. Hope that clarifies things!

Sure thing! That is already possible with these APIs: https://github.com/bellard/quickjs/blob/master/quickjs.h#L362

At this point I guess the question becomes about code size, but that is something LTO can help with.

[lanytcc]

I’m not entirely sure if enabling Bignum could impact the performance of operations on simple numbers.

It doesn't, AFAIK.

So, as a programmer, my first instinct is to turn off features that aren't necessary for my specific use case. Hope that clarifies things!

Sure thing! That is already possible with these APIs: https://github.com/bellard/quickjs/blob/master/quickjs.h#L362

At this point I guess the question becomes about code size, but that is something LTO can help with.

thanks for the heads up! I gotta admit, I overlooked those APIs. Appreciate you pointing them out to me. And good to know about the performance aspect not being affected by Bignum. Looks like I've got some reading to do

[past-due]

You could still do that by creating a raw context and not adding big int support, even though support for it could be builtin.

Yep, we already do this.

For anyone else who might be interested, here’s the custom JS_NewLimitedContext function (and related bits) we use:

Code example

```c typedef struct JSLimitedContextOptions { int baseObjects; int dateObject; int eval; int stringNormalize; int regExp; int json; int proxy; int mapSet; int typedArrays; int promise; int bigInt; } JSLimitedContextOptions; JSContext *JS_NewLimitedContext(JSRuntime *rt, const JSLimitedContextOptions* options) { JSContext *ctx; ctx = JS_NewContextRaw(rt); if (!ctx) return NULL; if (options->baseObjects) JS_AddIntrinsicBaseObjects(ctx); if (options->dateObject) JS_AddIntrinsicDate(ctx); if (options->eval) JS_AddIntrinsicEval(ctx); // required for JS_Eval (etc) to work if (options->stringNormalize) JS_AddIntrinsicStringNormalize(ctx); if (options->regExp) JS_AddIntrinsicRegExp(ctx); if (options->json) JS_AddIntrinsicJSON(ctx); if (options->proxy) JS_AddIntrinsicProxy(ctx); if (options->mapSet) JS_AddIntrinsicMapSet(ctx); if (options->typedArrays) JS_AddIntrinsicTypedArrays(ctx); if (options->promise) JS_AddIntrinsicPromise(ctx); if (options->bigInt) JS_AddIntrinsicBigInt(ctx); return ctx; } // Always accessible JS_Eval (even if limited context has eval disabled) JSValue JS_Eval_BypassLimitedContext(JSContext *ctx, const char *input, size_t input_len, const char *filename, int eval_flags) { int eval_type = eval_flags & JS_EVAL_TYPE_MASK; JSValue ret; assert(eval_type == JS_EVAL_TYPE_GLOBAL || eval_type == JS_EVAL_TYPE_MODULE); ret = __JS_EvalInternal(ctx, ctx->global_obj, input, input_len, filename, eval_flags, -1); return ret; } ``` Feel free to use under the same license terms as QuickJS itself.

(But we were also previously able to omit compiling libbf.c entirely, which was nice.)

We'll likely follow the same path with atomics, once support for Windows is added...

Keeping this stuff guarded by CONFIG_ATOMICS would be appreciated - not only because it currently relies on platform specifics, but also because it has effects beyond the Atomics support - such as added mutex usage to some places (like JS_NewClassID).

(We have embedded QuickJS with minimal patches and custom-configurable limited contexts running quite well across numerous platforms including Windows, Mac, BSDs, Linux, Emscripten, etc, compiling on GCC, Clang, and MSVC, and doing everything we need it to do. Works great! Here are the patches, in case there’s anything you might be interested in for upstreaming - most of it is platform / compiler compatibility bits: https://github.com/Warzone2100/quickjs-wz/tree/main/patches )

[saghul]

such as added mutex usage to some places (like JS_NewClassID).

I don't think those are in any hot paths. Also, FWIW we chose a different path in the friendly fork, without the need for locking.

[past-due]

Without getting too far off-topic, even if switching to <stdatomic.h>, it is recommended to check __STDC_NO_ATOMICS__, since atomics is an optional feature of the C11 standard.

__STDC_NO_ATOMICS__ The integer constant 1, intended to indicate that the implementation does not support atomic types (including the _Atomic type qualifier) and the <stdatomic.h> header.

Re: C 2018 6.10.8.3 1

So if there’s a desire to maintain the broad compatibility that the current code has (granted, with atomics disabled - but this is already done here for emscripten), presumably CONFIG_ATOMICS would be maintained to guard use of atomics (etc) anyway?

[saghul]

I'm currently only working on the friendly fork, and we haven't observed those problems. CONFIG_ATOMICS just guards the actual atomics API.

[chqrlie]

such as added mutex usage to some places (like JS_NewClassID).

I don't think those are in any hot paths. Also, FWIW we chose a different path in the friendly fork, without the need for locking.

We should use this approach too.

[chqrlie]

@lanytcc: I just committed a patch to fix the compilation error. I am not closing the issue yet because I want to address the other problems discussed here.

[Emill]

Currently the fix only works when compiling with optimizations turned on. Try make qjs-debug after removing CONFIG_BIGNUM=y in the Makefile and see that it fails.