Move debug dump constants to quickjs.h #770

mrmbernardi · 2024-12-25T15:01:57Z

Addresses #769

chqrlie

This does not quite meet the goal:

the debug flags are only defined for debug builds (#ifndef NDEBUG) and all of them get defined in this case.
the original design did not have the debug_flags tested by check_dump_flag and the debug code was only compiled and tested if the corresponding flags DUMP_XXX was defined. (I prefer this one, but we should check that the unused code is indeed removed).

The goal seems to be that external code can call the API JS_SetDumpFlags to select the appropriate debugging dumps using named flags instead of explicit hex values, provided the library was compiled with all of the defined.

Yet with this patch, the constants will only be defined and available for the external code if such code is itself compiled in debug mode, which may not be desirable.

An alternative seems advisable: define the JS_DUMP_XXX contants unconditionally and either:

use a different constant to select conditional compilation of the dump code and tests.
expand check_dump_flag to (0) in non debug builds and rely on the compiler / optimizer to omit the debugging code that becomes unreachable as if will only be referenced by if ((0)) blocks. This latter approach makes the code less clunky as the redundant #ifdef JS_DUMP_XXX / check_dump_flag(rt, JS_DUMP_XXX) get simplified.

In any case, either all debug handlers and dump code will be compiled or none of them. The granularity was finer before but this does not seem to be a problem as it was only useful because there was no way to control the output via a command line option or an API.

We should also make these changes:

make JS_SetDumpFlags return a boolean true in debug builds and false in regular builds
add another API JS_GetDumpFlags so temporary changes can be undone.

saghul · 2024-12-25T17:12:47Z

I agree we should likely define them unconditionally.

Then we can change the checks in the code to only run in not NDEBUG mode.

mrmbernardi · 2024-12-25T17:19:14Z

How about this:

Always define all JS_DUMP_XXX constants in quickjs.h
Change all #ifdef JS_DUMP_XXX in quickjs.c to #ifndef ENABLE_DUMPS or something.
By default debug builds set ENABLE_DUMPS

Maybe also only compile JS_SetDumpFlags with ENABLE_DUMPS so you'll get a linker error if you try to use it on an unsupported build?

saghul · 2024-12-25T17:45:14Z

That ENABLE_DUMPS is really NDEBUG negated

chqrlie · 2024-12-25T20:57:27Z

Maybe also only compile JS_SetDumpFlags with ENABLE_DUMPS so you'll get a linker error if you try to use it on an unsupported build?

I don't like this option because it prevents linking to a dynamic non-debug library, thus prevents other tests too

mrmbernardi · 2024-12-27T05:21:07Z

I made an option called ENABLE_DUMPS which is on by default on a debug build. All of the #ifdef statements now check ENABLE_DUMPS and I've given them comments to show which variables they were originally checking so its now easy to tell what dump feature they're servicing. I also made JS_SetDumpFlags return an int, -1 when ENABLE_DUMPS was not set on compilation, and 0 otherwise. This way you can get some feedback on whether dumping is supported when dynamically linking.

mrmbernardi · 2024-12-27T05:22:54Z

quickjs.c

-#if defined(DUMP_BYTECODE_FINAL) || \
-    defined(DUMP_BYTECODE_PASS2) || \
-    defined(DUMP_BYTECODE_PASS1) || \
-    defined(DUMP_BYTECODE_STACK) || \
-    defined(DUMP_BYTECODE_STEP)  || \
-    defined(DUMP_READ_OBJECT)
-#define DUMP_BYTECODE
-#endif


FYI I also found this section wasn't needed.

Yes, no longer needed, but somewhat confusing. It was used to trigger compilation of the bytecode dump utilities when one of the flags that needed it was defined. JS_DUMP_BYTECODE is no longer defined anywhere so the tests #ifdef ENABLE_DUMPS // JS_DUMP_BYTECODE may be misleading.

This was something I considered. I figured readers would be able to work out what's going on.

I changed the comments to JS_DUMP_BYTECODE_* to give a bit more of a hint.

chqrlie · 2024-12-27T09:09:56Z

quickjs.c

+int JS_SetDumpFlags(JSRuntime *rt, uint64_t flags)
 {
+#ifdef ENABLE_DUMPS
+    return 0;
    rt->dump_flags = flags;
+#else
+    return -1;
+#endif


This seems broken: the function returns before setting the flags

Oops. Thanks for catching that

chqrlie · 2024-12-27T09:12:22Z

quickjs.c

-#if defined(DUMP_BYTECODE_FINAL) || \
-    defined(DUMP_BYTECODE_PASS2) || \
-    defined(DUMP_BYTECODE_PASS1) || \
-    defined(DUMP_BYTECODE_STACK) || \
-    defined(DUMP_BYTECODE_STEP)  || \
-    defined(DUMP_READ_OBJECT)
-#define DUMP_BYTECODE
-#endif


Yes, no longer needed, but somewhat confusing. It was used to trigger compilation of the bytecode dump utilities when one of the flags that needed it was defined. JS_DUMP_BYTECODE is no longer defined anywhere so the tests #ifdef ENABLE_DUMPS // JS_DUMP_BYTECODE may be misleading.

chqrlie · 2024-12-27T09:16:18Z

quickjs.h

 JS_EXTERN void JS_SetMemoryLimit(JSRuntime *rt, size_t limit);
-JS_EXTERN void JS_SetDumpFlags(JSRuntime *rt, uint64_t flags);
+JS_EXTERN int JS_SetDumpFlags(JSRuntime *rt, uint64_t flags);
 JS_EXTERN size_t JS_GetGCThreshold(JSRuntime *rt);


Would you also add an API to get the current set of dump flags?

JS_EXTERN uint64_t JS_GetDumpFlags(JSRuntime *rt);

Added. However it seems the types are incorrect. Is it correct that the API sets and gets a uint64_t while the underlying variable is defined to be a 24 bit bit-field?

Maybe this is future proofing. The original library authors also seem to do this with JS_NewInt32 taking a context parameter when it's unused.

saghul · 2024-12-27T13:32:39Z

CMakeLists.txt

@@ -120,6 +120,11 @@ xoption(CONFIG_ASAN "Enable AddressSanitizer (ASan)" OFF)
 xoption(CONFIG_MSAN "Enable MemorySanitizer (MSan)" OFF)
 xoption(CONFIG_TSAN "Enable ThreadSanitizer (TSan)" OFF)
 xoption(CONFIG_UBSAN "Enable UndefinedBehaviorSanitizer (UBSan)" OFF)
+if(CMAKE_BUILD_TYPE MATCHES "Debug")
+    xoption(ENABLE_DUMPS "Enable Dumps" ON)


I'd rather not add a new definition NDEBUG is already a thing.

The intent is to allow dumps in release builds or create debug builds without the dump code. IMO this can be quite valuable when debugging issues relating to the dump code. The defaults handle the typical case.

I think that's a separate change from what this PR started as. We can take one step at a time.

Okay I reverted this change

saghul · 2024-12-27T13:33:28Z

quickjs.h

@@ -327,7 +350,8 @@ JS_EXTERN JSRuntime *JS_NewRuntime(void);
 JS_EXTERN void JS_SetRuntimeInfo(JSRuntime *rt, const char *info);
 /* use 0 to disable memory limit */
 JS_EXTERN void JS_SetMemoryLimit(JSRuntime *rt, size_t limit);
-JS_EXTERN void JS_SetDumpFlags(JSRuntime *rt, uint64_t flags);
+JS_EXTERN int JS_SetDumpFlags(JSRuntime *rt, uint64_t flags);


We could leave it as it was and make it a noop in release buids.

Should users be able to tell if they've linked to a build that supports dumping?

Currently, due to the very specific nature of it, the user knows for sure.

Changed to noop

chqrlie · 2024-12-30T12:41:51Z

quickjs.c

-
-#ifndef NDEBUG
-#define DUMP_BYTECODE_FINAL   0x01  /* dump pass 3 final byte code */
-#define DUMP_BYTECODE_PASS2   0x02  /* dump pass 2 code */


Using a specific define ENABLE_DUMPS was fine and preferreable over using #ifndef NDEBUG everywhere.
I suppose saghul did not want it defined in the CMakefile.txt. You should define here this way:

#ifndef NDEBUG #define ENABLE_DUMPS #endif

This allows temporary definition to investigate problems in the release build.

Sorry for the confusion and the extra work.

No worries. I was on break for a bit but got it sorted now.

mrmbernardi force-pushed the fix-769 branch from e6cf128 to 3664c01 Compare December 25, 2024 15:39

chqrlie requested changes Dec 25, 2024

View reviewed changes

mrmbernardi force-pushed the fix-769 branch 4 times, most recently from 628032b to a5a1d47 Compare December 27, 2024 05:20

mrmbernardi commented Dec 27, 2024

View reviewed changes

chqrlie requested changes Dec 27, 2024

View reviewed changes

mrmbernardi force-pushed the fix-769 branch from a5a1d47 to 39c3a13 Compare December 27, 2024 12:46

saghul reviewed Dec 27, 2024

View reviewed changes

mrmbernardi force-pushed the fix-769 branch 2 times, most recently from 1d8bfca to 7189236 Compare December 29, 2024 07:07

chqrlie requested changes Dec 30, 2024

View reviewed changes

chqrlie mentioned this pull request Dec 30, 2024

expose the bytecode in cli #777

Open

mrmbernardi force-pushed the fix-769 branch from 7189236 to 5a3c79c Compare January 11, 2025 16:59

Move debug dump constants to quickjs.h and add ENABLE_DUMPS switch

fa02fd2

mrmbernardi force-pushed the fix-769 branch from 5a3c79c to fa02fd2 Compare January 11, 2025 17:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move debug dump constants to quickjs.h #770

Move debug dump constants to quickjs.h #770

mrmbernardi commented Dec 25, 2024

chqrlie left a comment •

edited

Loading

saghul commented Dec 25, 2024

mrmbernardi commented Dec 25, 2024 •

edited

Loading

saghul commented Dec 25, 2024

chqrlie commented Dec 25, 2024

mrmbernardi commented Dec 27, 2024

mrmbernardi Dec 27, 2024

chqrlie Dec 27, 2024

mrmbernardi Dec 27, 2024

chqrlie Dec 27, 2024

mrmbernardi Dec 27, 2024

chqrlie Dec 27, 2024

chqrlie Dec 27, 2024

mrmbernardi Dec 27, 2024

saghul Dec 27, 2024

mrmbernardi Dec 27, 2024

saghul Dec 27, 2024

mrmbernardi Dec 29, 2024

saghul Dec 27, 2024

mrmbernardi Dec 27, 2024

saghul Dec 27, 2024

mrmbernardi Dec 29, 2024

chqrlie Dec 30, 2024

mrmbernardi Jan 11, 2025

Move debug dump constants to quickjs.h #770

Are you sure you want to change the base?

Move debug dump constants to quickjs.h #770

Conversation

mrmbernardi commented Dec 25, 2024

chqrlie left a comment • edited Loading

Choose a reason for hiding this comment

saghul commented Dec 25, 2024

mrmbernardi commented Dec 25, 2024 • edited Loading

saghul commented Dec 25, 2024

chqrlie commented Dec 25, 2024

mrmbernardi commented Dec 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chqrlie left a comment •

edited

Loading

mrmbernardi commented Dec 25, 2024 •

edited

Loading