Sync from Moddable 3.9.2 to 5.5.0 #58

kriskowal · 2025-04-17T23:26:03Z

No description provided.

kriskowal · 2025-04-17T23:28:25Z

xsnap/makefiles/mac/xsnap-worker.mk

+## present in 3.9.2 xst.mk
+## C_OPTIONS += -fsanitize=address -fno-omit-frame-pointer
+## LINK_OPTIONS += -fsanitize=address -fno-omit-frame-pointer
+# present in 5.5.0 xst.mk
+#	ifeq ($(SANITIZER), undefined)
+#		C_OPTIONS += -fsanitize=bool,builtin,enum,integer-divide-by-zero,null,object-size,return,returns-nonnull-attribute,shift,signed-integer-overflow,unreachable,vla-bound,vptr -fno-sanitize-recover=bool,builtin,enum,integer-divide-by-zero,null,object-size,return,returns-nonnull-attribute,shift,signed-integer-overflow,unreachable,vla-bound,vptr,array-bounds,function
+#		LINK_OPTIONS += -fsanitize=bool,builtin,enum,integer-divide-by-zero,null,object-size,return,returns-nonnull-attribute,shift,signed-integer-overflow,unreachable,vla-bound,vptr -fno-sanitize-recover=bool,builtin,enum,integer-divide-by-zero,null,object-size,return,returns-nonnull-attribute,shift,signed-integer-overflow,unreachable,vla-bound,vptr,array-bounds,function
+#	else
+#		C_OPTIONS += -fsanitize=address -fsanitize-blacklist=xst_no_asan.txt
+#		LINK_OPTIONS += -fsanitize=address
+#	endif
+#	C_OPTIONS += -DmxASANStackMargin=131072 -fno-omit-frame-pointer
+#	LINK_OPTIONS += -fno-omit-frame-pointer


Please confirm I can safely delete this commented section that I found in xst.mk on Moddable 5.5.0.

That all appears to be related to fuzzer configurations. You should be able to report that.

FWIW – it might be interesting to run xsnap with sanitizers enabled in a debug / development environment. It certainly uses XS in ways that are different from the fuzzers and would also check your name code. That should be an option since the performance impact of the sanitizers is considerable.

kriskowal · 2025-04-17T23:29:04Z

xsnap/makefiles/mac/xsnap-worker.mk

+ 	$(TMP_DIR)/e_acos.o \
+ 	$(TMP_DIR)/e_acosh.o \
+ 	$(TMP_DIR)/e_asin.o \
+ 	$(TMP_DIR)/e_atan2.o \
+ 	$(TMP_DIR)/e_atanh.o \
+ 	$(TMP_DIR)/e_cosh.o \
+ 	$(TMP_DIR)/e_exp.o \
+ 	$(TMP_DIR)/e_hypot.o \
+ 	$(TMP_DIR)/e_log.o \
+ 	$(TMP_DIR)/e_log10.o \
+ 	$(TMP_DIR)/e_pow.o \
+ 	$(TMP_DIR)/e_rem_pio2.o \
+ 	$(TMP_DIR)/e_sinh.o \
+ 	$(TMP_DIR)/k_cos.o \
+ 	$(TMP_DIR)/k_exp.o \
+ 	$(TMP_DIR)/k_rem_pio2.o \
+ 	$(TMP_DIR)/k_sin.o \
+ 	$(TMP_DIR)/k_tan.o \
+ 	$(TMP_DIR)/s_asinh.o \
+ 	$(TMP_DIR)/s_atan.o \
+ 	$(TMP_DIR)/s_cos.o \
+ 	$(TMP_DIR)/s_expm1.o \
+ 	$(TMP_DIR)/s_log1p.o \
+ 	$(TMP_DIR)/s_scalbn.o \
+ 	$(TMP_DIR)/s_sin.o \
+ 	$(TMP_DIR)/s_tan.o \
+ 	$(TMP_DIR)/s_tanh.o \


These are not present in lin/xsnap-worker.mk. Should we sync these from Mac to Linux?

kriskowal · 2025-04-17T23:30:00Z

xsnap/makefiles/mac/xsnap-worker.mk

 	$(TMP_DIR)/xsnap-worker.o

-VPATH += $(SRC_DIR) $(TLS_DIR)
+VPATH += $(SRC_DIR) $(TLS_DIR) $(XS_TLS_DIR)/fdlibm


This is only slightly different than what I found in xst.mk. The difference accounts for xsnap tools being a different than xs tools.

kriskowal · 2025-04-17T23:32:58Z

xsnap/sources/xsnap-worker.c

+					// TODO reason through this follow-up from moddable's fork between 3.9.2 and 5.5.0
+					//path = nsbuf + 1;
+					//snapshot.stream = fopen(path, "rb");
+					//if (snapshot.stream) {
+					//	fxUseSnapshot(machine, &snapshot);
+					//	fclose(snapshot.stream);
+					//}
+					//else
+					//	snapshot.error = errno;
+					//if (snapshot.error) {
+					//	fprintf(stderr, "cannot restore snapshot %s: %s\n",
+					//			path, strerror(snapshot.error));
+					//	c_exit(E_IO_ERROR);
+					//}


This section is present in the Moddable fork of xsnap-worker.c. It looks like it’s verifying that the snapshot can round-trip back. I found that some unit tests in agoric-sdk/packages/xsnap failed with this code in place. This may simply have been because the path was a file descriptor reference. If it’s useful, I could make this contingent on whether path starts with @.

attn @mhofman

Ah yeah this was added by Moddable to reset the machine from snapshot to avoid performance degradation due to memory fragmentation. We do not need this as we do this upstream, and it is actually harmful to have that behavior hardcoded as we need the ability to test without reloading to detect load from snapshot divergence bugs.

Please take a look at discarded changes from #44 compared to what was merged in #46

Also see #48 for a proposal to make this use snapshot explicit.

We never landed it because we instead found it more efficient to start another worker and stream the snapshot to it (the agoric process is teeing the snapshot stream into both the swing-store and to the new worker).

kriskowal · 2025-04-17T23:37:26Z

xsnap/sources/xsnap.c

+						// TODO reconcile with xnsap-worker.c, which has a try/catch
+						// {
+						//	xsVars(3);
+						//	xsTry {
+						//		if (command == '?') {
+						//			#if XSNAP_TEST_RECORD
+						//				fxTestRecord(mxTestRecordJSON | mxTestRecordParam, nsbuf + 1, nslen - 1);
+						//			#endif
+						//			// TODO: can we avoid a copy?
+						//			xsVar(0) = xsArrayBuffer(nsbuf + 1, nslen - 1);
+						//			xsVar(1) = xsCall1(xsGlobal, xsID("handleCommand"), xsVar(0));
+						//		} else {
+						//			#if XSNAP_TEST_RECORD
+						//				fxTestRecord(mxTestRecordJS | mxTestRecordParam, nsbuf + 1, nslen - 1);
+						//			#endif
+						//			// TODO reconcile with xsnap.c -e
+						//			// xsResult = xsString(argv[argi]);
+						//			// xsCall1(xsGlobal, xsID("eval"), xsResult);
+						//			xsVar(0) = xsStringBuffer(nsbuf + 1, nslen - 1);
+						//			xsVar(1) = xsCall1(xsGlobal, xsID("eval"), xsVar(0));
+						//		}
+						//	}
+						//	xsCatch {
+						//		if (xsTypeOf(xsException) != xsUndefinedType) {
+						//			// fprintf(stderr, "%c: %s\n", command, xsToString(xsException));
+						//			error = E_UNHANDLED_EXCEPTION;
+						//			xsVar(1) = xsException;
+						//			xsException = xsUndefined;
+						//		}
+						//	}
+						// }


I intend to delete this note. I made some early attempt to reconcile xsnap.c and xsnap-worker.c, and this was a note to that effect.

kriskowal · 2025-04-17T23:41:21Z

xsnap/sources/xsnapPlatform.h

+// XXX present in xst.h but absent on my machine. system dependency? need linkage in Makefile?
+//#include <fdlibm.h>


I think this is safe to delete.

I think it's in the makefile, right?

mhofman

My biggest concerns are around the scope of the change for timers which seem to involves a bunch of mutex logic, and the apparent addition of some I/O in module loading.

mhofman · 2025-04-18T00:00:24Z

xsnap/makefiles/mac/xsnap-worker.mk

+$(OBJECTS): $(XS_TLS_DIR)/fdlibm/math_private.h
 $(OBJECTS): $(SRC_DIR)/xsSnapshot.h
 $(OBJECTS): $(INC_DIR)/xs.h
-$(OBJECTS): $(EXTRA_DEPS)


This is an agoric-sdk build mechanism to force rebuild on config changes, please do not remove.

mhofman · 2025-04-18T00:06:00Z

xsnap/sources/xsnap-worker.c

+					// TODO reason through this follow-up from moddable's fork between 3.9.2 and 5.5.0
+					//path = nsbuf + 1;
+					//snapshot.stream = fopen(path, "rb");
+					//if (snapshot.stream) {
+					//	fxUseSnapshot(machine, &snapshot);
+					//	fclose(snapshot.stream);
+					//}
+					//else
+					//	snapshot.error = errno;
+					//if (snapshot.error) {
+					//	fprintf(stderr, "cannot restore snapshot %s: %s\n",
+					//			path, strerror(snapshot.error));
+					//	c_exit(E_IO_ERROR);
+					//}


Ah yeah this was added by Moddable to reset the machine from snapshot to avoid performance degradation due to memory fragmentation. We do not need this as we do this upstream, and it is actually harmful to have that behavior hardcoded as we need the ability to test without reloading to detect load from snapshot divergence bugs.

Please take a look at discarded changes from #44 compared to what was merged in #46

mhofman · 2025-04-18T00:11:05Z

xsnap/makefiles/lin/xsnap-worker.mk

 	-DmxMetering=1 \
 	-DmxDebug=1 \
-	-UmxInstrument \
+	-DmxInstrument=1 \


I believe this used to cause issues, or at least I wasn't confident in its behavior. See #34

As a rule mxInstrument and mxDebug are checked to see if they defined. The value shouldn't matter. That said, I prefer setting it to 1. But, whatever you do, don't set it 0 which suggests the feature is disabled when it isn't.

mxInstrument is indeed still problematic and I’ve restored the -U flag just to be sure.

mhofman · 2025-04-18T00:11:23Z

xsnap/makefiles/lin/xsnap-worker.mk

 $(OBJECTS): $(SRC_DIR)/xsScript.h
 $(OBJECTS): $(SRC_DIR)/xsSnapshot.h
 $(OBJECTS): $(INC_DIR)/xs.h
-$(OBJECTS): $(EXTRA_DEPS)


This is an agoric-sdk build mechanism to force rebuild on config changes, please do not remove.

mhofman · 2025-04-18T00:17:18Z

xsnap/sources/xsnap-worker.c

 	char prefix[8 + sizeof fmt + 8 * sizeof numeral64 + sizeof timestampBuffer];
 	// Prepend the meter usage to the reply.
-	snprintf(prefix, sizeof(prefix), fmt,
+	snprintf(prefix, sizeof(prefix) - 1, fmt,


Do we need to make prefix 1 bigger instead?

That’s reasonable. I believe sprintfn assumes a space for the null terminator beyond the length.

The snprintf() and vsnprintf() functions will write at most size-1 of the characters printed into the output string (the size'th character then gets the terminating ‘\0’); if the return value is greater than or equal to the size argument, the string was too short and some of the printed characters were discarded. The output is always null-terminated, unless size is 0.

mhofman · 2025-04-18T00:30:51Z

xsnap/sources/xsnapPlatform.c

+		#if mxWindows
+			DWORD attributes;
+			attributes = GetFileAttributes(path);
+			if (attributes != 0xFFFFFFFF) {
+				if (attributes & FILE_ATTRIBUTE_DIRECTORY)
+					return;
+			}
+		#else
+			struct stat a_stat;
+			if (stat(path, &a_stat) == 0) {
+				if (S_ISDIR(a_stat.st_mode))
+					return;
+			}
+		#endif


I thought we had nerfed file system access. Slightly concerned we'd be doing I/O possibly before the nerfing point.

mhofman · 2025-04-18T00:31:46Z

xsnap/sources/xsnapPlatform.c

+	FILE* file;
+};
+
+typedef void (*txDumpChunk)(txDumper* dumper, txByte* data, txSize size);


Yay for abstractions

mhofman · 2025-04-18T00:33:00Z

xsnap/sources/xsnapPlatform.h

+// XXX present in xst.h but absent on my machine. system dependency? need linkage in Makefile?
+//#include <fdlibm.h>


I think it's in the makefile, right?

mhofman · 2025-04-18T00:34:35Z

xsnap/sources/xsnapPlatform.h

+  	#if GNUC > 11
+  		#define mxUseFloat16 1
+  	#endif


Do we need to require a minimum version of the compiler to make sure we don't end up with different representations floating around. I believe we already have some checks like that for hasBuiltin

It the compiler supports float16 it is IEEE numerics by definition, so there shouldn't be variation in the representations.

My concern is if we allowed a mix of compiler that either had this enabled or not. I think we should just fail to compile if we can't enable this?

You could. But, it is unnecessary. XS contains the necessary fallback code for when the compiler does not support float16 natively. That has been verified with test262.

mhofman · 2025-04-18T00:36:20Z

xsnap/sources/xsnapPlatform.h

+    // XXX xst.h also has:
+ 	  // void* script;		// txScript*


I suspect this has something to do with runScript ?

mhofman · 2025-04-18T00:40:20Z

Oh and as discussed offline, making sure we use fdlibm on linux. Definitely want to take advantage there.

kriskowal · 2025-04-30T00:08:31Z

I’ve, among other things,

ported the fdlibm changes to the linux makefile
sorted out the timers. we’re not going to support shared timers, but the hooks have to be present in the platform, so i’ve left noop for the ones that can’t be avoided and panic from the ones that shouldn’t be called.
gotten this to pass all xsap tests in agoric-sdk locally, so far

kriskowal · 2025-09-08T21:37:31Z

This work is not a current priority for Agoric, but if we can review and merge this in the occasional spare cycle, I think we’ll be better off overall than letting these bits rot. Please consider a review.

kriskowal force-pushed the kriskowal-5.5.0 branch from a4c4be9 to d793ca0 Compare April 17, 2025 23:26

kriskowal commented Apr 17, 2025

View reviewed changes

kriskowal requested a review from phoddie April 17, 2025 23:41

Sync from Moddable 3.9.2 to 5.5.0

082a2f7

kriskowal force-pushed the kriskowal-5.5.0 branch from d793ca0 to 082a2f7 Compare April 17, 2025 23:45

mhofman reviewed Apr 18, 2025

View reviewed changes

kriskowal added 5 commits April 24, 2025 16:15

fixup timers

c5a4f30

progress

c6b9f5f

debug

b127f01

progress

caa98f0

realign lin with mac makefile for fdlibm

39570cd

kriskowal added 3 commits May 5, 2025 17:21

WIP withdraw spurious comment

c9b2e4e

Linux build fix

5462327

WIP include xsnapPlatform.h

10c07a3

kriskowal force-pushed the kriskowal-5.5.0 branch from 8f575d7 to 10c07a3 Compare May 6, 2025 00:37

kriskowal added 8 commits May 6, 2025 13:49

portable printf int type

bc45fd0

dladdr binding on linux remains illusive

0f2cad2

maybe shared timers hook getting called

7bbef56

return null

680af92

ah, C_NULL

ad7f25a

no null for you

e0d06b5

no key gc

365aeaf

harmonize allocators

48cc664

mhofman mentioned this pull request Jul 30, 2025

refactor: Improve and align Makefiles #50

Open

kriskowal marked this pull request as ready for review September 8, 2025 21:36

		// XXX present in xst.h but absent on my machine. system dependency? need linkage in Makefile?
		//#include <fdlibm.h>

Sync from Moddable 3.9.2 to 5.5.0 #58

Are you sure you want to change the base?

Sync from Moddable 3.9.2 to 5.5.0 #58

Uh oh!

Conversation

kriskowal commented Apr 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhofman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhofman commented Apr 18, 2025

Uh oh!

kriskowal commented Apr 30, 2025

Uh oh!

kriskowal commented Sep 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants