Fix signedness/type mismatches when marshaling stat-related structs

[kleisauke]

No description provided.

[sbc100]

lgtm, but I'm fine with #24769 landing first too.

[sbc100]

IIRC the signed-ness of the type only really matter for makeGetValue.. is that right? i.e. its when reading the bits that its important to interpret them.

Supporting evidence for this is that the makeSetValue code doesn't even have code paths for handling u64 and u53 here:

emscripten/src/parseTools.mjs

Lines 453 to 466 in 90250aa

	if (type == 'i64' && !WASM_BIGINT) {
	// If we lack BigInt support we must fall back to an reading a pair of I32
	// values.
	// prettier-ignore
	return '(tempI64 = [' + splitI64(value) + '], ' +
	makeSetValueImpl(ptr, pos, 'tempI64[0]', 'i32') + ',' +
	makeSetValueImpl(ptr, getFastValue(pos, '+', getNativeTypeSize('i32')), 'tempI64[1]', 'i32') + ')';
	}
	const offset = calcFastOffset(ptr, pos);
	if (type === 'i53') {
	return `writeI53ToI64(${offset}, ${value})`;
	}

I think maybe just always use the signed array views works for setting values?

I'm not sure what happens if we write a negative number to an unsigned typed array, but I guess its undefined behaviour from our POV and we should assert in debug build.

[kleisauke]

According to the failing tests, there's seems to be indeed support missing for -sWASM_BIGINT=0 and makeSetValue(x, y, z, 'u64'). Commit 5511235 reverts that part.

I'm not sure if this signedness mismatch only affects makeGetValue(), since getHeapForType() is also used in makeSetValue() (e.g. HEAP32 versus HEAPU32).

[sbc100]

I'm not sure if this signedness mismatch only affects makeGetValue(), since getHeapForType() is also used in makeSetValue() (e.g. HEAP32 versus HEAPU32).

I'm not sure either, but I would say that seems like signedness is critical when reading values (since the bytes need to be interpreted), but maybe not critical when writing them.

[kleisauke]

The codesize tests appear to be quite sensitive, especially other.test_codesize_hello_dylink_all. IIRC, we allowed some slop/slack factor in the past, but it seems that's no longer configurable.

[sbc100]

Yes, I apologize for that. I'm not sure if we should remove that test again.. maybe? Or we should have it auto-update as part of the review process.

[kleisauke]

I'm not certain what the ideal solution is, but one possibility is to create a bot that automatically updates test expectations whenever a specific label (perhaps reuse code size or introduce "needs-rebaseline"?) is applied to a PR. Once the bot completes the update, it would remove the label.

Using labels helps prevent potential abuse, since applying them requires triage access or higher (this idea is also somewhat inspired by LLVM's convenient /cherry-pick command, which also requires proper permissions, such as the ability to edit the milestone).

[kleisauke]

... ah, I forgot that this only works when the "Allow edits by maintainers" option is enabled, though I believe it's enabled by default.

[sbc100]

I'm not certain what the ideal solution is, but one possibility is to create a bot that automatically updates test expectations whenever a specific label (perhaps reuse code size or introduce "needs-rebaseline"?) is applied to a PR. Once the bot completes the update, it would remove the label.

Using labels helps prevent potential abuse, since applying them requires triage access or higher (this idea is also somewhat inspired by LLVM's convenient /cherry-pick command, which also requires proper permissions, such as the ability to edit the milestone).

Yes, we already have an action that will create rebasline CLs for main: https://github.com/emscripten-core/emscripten/actions/workflows/rebaseline-tests.yml

Figuring out how to make this work PRs in flight is what I'd like to do.

I've not get written a bot that responds to labels of commands but it seems possible.