Skip to content

"fix" segv from #3198, #3199, and #3205#3207

Merged
mthom merged 9 commits into
mthom:masterfrom
Skgland:quote-fix-unquote-segv
Dec 17, 2025
Merged

"fix" segv from #3198, #3199, and #3205#3207
mthom merged 9 commits into
mthom:masterfrom
Skgland:quote-fix-unquote-segv

Conversation

@Skgland
Copy link
Copy Markdown
Contributor

@Skgland Skgland commented Dec 7, 2025

Fix in quotes as the first two are only converted to panics instead of a full fix.

Issues #3198, #3199 & #3205 are partially fixed/improved.

@Skgland
Copy link
Copy Markdown
Contributor Author

Skgland commented Dec 7, 2025

Uh, changing the heap_index! macro to use checked multiplication is quite bad (x2) for speed.

@Skgland
Copy link
Copy Markdown
Contributor Author

Skgland commented Dec 7, 2025

🤦‍♂️ eagerly formatting the error message was the reason for the slow down.
Switching to lazy formatting brings the speed back to about the prior level.

@Skgland
Copy link
Copy Markdown
Contributor Author

Skgland commented Dec 7, 2025

This should now be ready for review.

@Skgland
Copy link
Copy Markdown
Contributor Author

Skgland commented Dec 8, 2025

Still ready for review, just with an additional overflow fixed.

Comment thread src/machine/machine_errors.rs Outdated
Copy link
Copy Markdown
Contributor

@bakaq bakaq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@UWN
Copy link
Copy Markdown

UWN commented Dec 9, 2025

;  E = 64, N = 18446744073709551616, Er = resource_error(finite_memory,18446744073709551616)

Why is the second argument now the size in question? This does not make much sense. Apart from finite_. In particular cumbersome for much larger values.

@UWN
Copy link
Copy Markdown

UWN commented Dec 9, 2025

... that is, why is there a second argument at all? The term should be resource_error(memory).

@Skgland
Copy link
Copy Markdown
Contributor Author

Skgland commented Dec 9, 2025

;  E = 64, N = 18446744073709551616, Er = resource_error(finite_memory,18446744073709551616)

Why is the second argument now the size in question? This does not make much sense. Apart from finite_. In particular cumbersome for much larger values.

As I said in #3205 (comment)
for larger E this runs into #3201 which I didn't attempt to fix here in this PR.

Basically for E>=64 an allocation in a different code path converting N to usize fails which has a pre-existing bug of emitting the wrong error.

@UWN
Copy link
Copy Markdown

UWN commented Dec 9, 2025

which I didn't attempt to fix here in this PR.

Why not. Without it testing becomes cumbersome.

@Skgland
Copy link
Copy Markdown
Contributor Author

Skgland commented Dec 9, 2025

which I didn't attempt to fix here in this PR.

Why not. Without it testing becomes cumbersome.

Multiple reasons:

@UWN
Copy link
Copy Markdown

UWN commented Dec 9, 2025

(exact form up for debate/bikeshedding)

Only error(resource_error(memory),[]) does not entail further problems. In any case, I will have to wait for further testing.

@Skgland
Copy link
Copy Markdown
Contributor Author

Skgland commented Dec 9, 2025

I think further discussion regarding the resource_error(finite_memory,_) should be moved to #3201 and not be held in this PR.

@Skgland Skgland marked this pull request as draft December 9, 2025 22:59
@Skgland Skgland marked this pull request as ready for review December 9, 2025 23:04
@Skgland
Copy link
Copy Markdown
Contributor Author

Skgland commented Dec 9, 2025

@UWN this now includes a fix for the misleading error from #3201.

The change was sufficiently small and while context would be nice (to differentiate "reasonable" amounts of missing memory from architecture exceeding amounts) it is not required.

With that, this is again ready for review.

@UWN
Copy link
Copy Markdown

UWN commented Dec 10, 2025

Solves #3205, #3201 (I read: Changes reviewed No applicable reviews submitted by reviewers with write access.)

@UWN UWN mentioned this pull request Dec 10, 2025
@UWN
Copy link
Copy Markdown

UWN commented Dec 12, 2025

I found no problems. This is ready for merging. Thx!

@mthom mthom merged commit 172b6a6 into mthom:master Dec 17, 2025
13 checks passed
@Skgland Skgland deleted the quote-fix-unquote-segv branch December 17, 2025 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants