Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"0" and "@" hash value conflicted? #1209

Closed
linfan68 opened this issue Sep 12, 2023 · 9 comments
Closed

"0" and "@" hash value conflicted? #1209

linfan68 opened this issue Sep 12, 2023 · 9 comments
Labels
fixed - please verify Issue has been fixed. Please verify and close.

Comments

@linfan68
Copy link

It seems like if I put "@" into an JSON object, it's hash value got conflicted with "0"? Very interesting

from helloworld project:

debugger;

let message = "Hello, world - sample";
const chMap = {
  "0": 48,
  "@": 64,
}
trace(message + "\n\n");
trace('[0] of chMap is :' + chMap['0'].toString() + '\n\n');

trace(chMap['@'].toString() + '\n'); // This crashes

Expected output is :

Hello, world - sample

[0] of chMap is :48

64

But I got

Hello, world - sample

[0] of chMap is :64

XS abort: unhandled exception
@phoddie
Copy link
Collaborator

phoddie commented Sep 12, 2023

Very strange bug. Thanks for reporting it.

chMap = {"@": 64} shows the problem most clearly. In that case, chMap contains {"0":64} which is obviously incorrect.

Doing chMap = {}; chMap["@"] = 64 works as expected, setting chMap to {"@":64}.

That can co-exist with 0. chMap = {"0": 48}; chMap["@"] = 64 results in {"0":48,"@":64}.

That suggests that the problem is in the parsing of chMap = {"@": 64} and not the runtime implementation of the object.

FWIW – you see "XS abort: unhandled exception" because chMap['@'] evaluates to undefined and undefined.toString() throws an exception in JavaScript. If you write it as String(chMap['@']) it evaluates to "undefined". That's just JavaScript trivia. ;)

We'll look into the bug

@phoddie
Copy link
Collaborator

phoddie commented Sep 13, 2023

The test case works correctly in xst. So, it isn't parsing. It turns out the problem is in the linker. The fix is to change...

linker->symbolTable[aModulo] = aSymbol;

...to...

		if (anID >= XS_SYMBOL_ID_COUNT)
			linker->symbolTable[aModulo] = aSymbol;

The problem would also occur with built-in symbols like Symbol.asyncDispose. Good find. Thanks again. Glad we got this fixed.

(The patch above will be merged to this repository in the coming days.)

@linfan68
Copy link
Author

The test case works correctly in xst. So, it isn't parsing. It turns out the problem is in the linker. The fix is to change...

linker->symbolTable[aModulo] = aSymbol;

...to...

		if (anID >= XS_SYMBOL_ID_COUNT)
			linker->symbolTable[aModulo] = aSymbol;

The problem would also occur with built-in symbols like Symbol.asyncDispose. Good find. Thanks again. Glad we got this fixed.

(The patch above will be merged to this repository in the coming days.)

I've verified with a local code change and it works. I'll close this issue, thanks!

@phoddie
Copy link
Collaborator

phoddie commented Sep 18, 2023

Thank you for confirming. The fix is live in this repository now. We really appreciate your attention to detail in finding this issue.

@linfan68
Copy link
Author

@phoddie Can you verify this is fixed?
I just updated to latest public (4.3.0), the code looks like this:

		aSymbol->flag = flag;
		linker->symbolArray[anID] = aSymbol;
		if (table)
			linker->symbolTable[aModulo] = aSymbol;
		linker->symbolIndex++;

And the problem found in my code still exists.
After changing to:

		aSymbol->flag = flag;
		linker->symbolArray[anID] = aSymbol;
		if (table)
			if (anID >= XS_SYMBOL_ID_COUNT)
				linker->symbolTable[aModulo] = aSymbol;
		linker->symbolIndex++;

It got fixed.

@linfan68 linfan68 reopened this Nov 18, 2023
@phoddie
Copy link
Collaborator

phoddie commented Nov 19, 2023

It looks like one fix stepped on another. Sorry about that. Your merge of the two fixes does seem to work. We'll review that and update.

@phoddie
Copy link
Collaborator

phoddie commented Nov 19, 2023

Just FYI – merging the two changes works on a clean build, but can fail on an incremental build. We have a fix that should always work. That will be live soon.

Thanks for reporting this issue, again.

mkellner pushed a commit that referenced this issue Nov 24, 2023
@phoddie
Copy link
Collaborator

phoddie commented Nov 24, 2023

This should be better now. Please try with the latest. Thank you,

@phoddie phoddie added the fixed - please verify Issue has been fixed. Please verify and close. label Nov 24, 2023
@phoddie
Copy link
Collaborator

phoddie commented Dec 23, 2023

Closing (fixed).

@phoddie phoddie closed this as completed Dec 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed - please verify Issue has been fixed. Please verify and close.
Projects
None yet
Development

No branches or pull requests

2 participants