Start addressing some bugs in new Rust code #10

trurl · 2025-11-29T14:40:08-05:00

trurl commented

2025-11-29 14:40:08 -05:00

The Rust replacements for WMHashTable and WMArray cause segfaults when called into from existing C code. This is largely due to deficiencies on my part. (The WMHashTable rewrite was committed prematurely, and I seem to have misunderstood what the signature of callbacks passed to the WMArray interface should be.) This PR addresses some of these problems.

trurl added 2 commits 2025-11-29 14:40:10 -05:00

Store addresses insead of pointers in WMArray. 5ec46b217d

Prior to this patch, when WINGs/Tests/wtest.c tries to build a FontPanel, it
crashes. This is because the rewritten WMSortArray passes pointers to array
items to its comparator function, but the original API passed pointers to
pointers to array items to its comparator function. This has been corrected, and
now WMSortArray should behave more like it originally did.

This patch also stores `usize` addresses instead of `NonNull` pointers because
some WMArray use sites actually store non-pointer data (by casting it to
uintptr_t or similar).

Fix omissions in WMHashEnumerator C interface. b39e16d6b2

The WMHashTable rewrite branch was prematurely merged. This patch fixes some of
the things that were overlooked in that merge. WMHashEnumerator should be an
opaque type that is free'd after it is created.

We hope to axe WMHashTable entirely, but it's reasonable to get this fix in for
now so that the WMFontPanel integration test in WINGs/Tests can run on top of
the changes we've made so far. As of this commit, it's still broken, but it
behaves better than it did.

cross reviewed 2025-12-02 13:58:55 -05:00

wutil-rs/src/array.rs Outdated

						
				@@ -226,7 +218,7 @@ pub mod ffi {

				            (*array)

				                .items

				                .pop()

				                .map(|p| p.as_ptr())

cross commented

2025-12-02 13:58:55 -05:00

Could you use .cast() here instead of as (and similarly elsewhere)?

Could you use `.cast()` here instead of `as` (and similarly elsewhere)?

trurl commented

2025-12-08 13:53:04 -05:00

I'm not sure I can do this. Is there a .cast() method for converting between raw pointer and non-raw-pointer types? For example, most of the time this file uses as, it's casting a usize to some flavor of void*.

I'm not sure I can do this. Is there a `.cast()` method for converting between raw pointer and non-raw-pointer types? For example, most of the time this file uses `as`, it's casting a `usize` to some flavor of `void*`.

cross commented

2025-12-12 07:31:51 -05:00

oh, hmm. No, not really, then. In that case, I'd recommend using the "provenance" based functions, though: .with_addr and so forth, if that's reasonable: https://doc.rust-lang.org/std/ptr/index.html#strict-provenance

oh, hmm. No, not really, then. In that case, I'd recommend using the "provenance" based functions, though: `.with_addr` and so forth, if that's reasonable: https://doc.rust-lang.org/std/ptr/index.html#strict-provenance

trurl commented

2025-12-15 12:16:50 -05:00

I don't see how I can reasonably use with_addr to retain provenance when converting values stored in the array from usize back to pointers. The problem lies in how this API is used by existing code: the caller may provide arbitrary integers to functions like WMInsertInArray. When those values are queried by C code, they may be cast back to integers or dereferenced. The knowledge of whether the value is a real address with proper provenance is only in the programmer's head. As you have suggested, I'd have to track something at runtime or split this into two different array types (one for usizes, one for * mut c_voids).

I do hope to be rid of WMArray sooner or later, so for now I think I'll just keep the casts as-is.

I don't see how I can reasonably use `with_addr` to retain provenance when converting values stored in the array from `usize` back to pointers. The problem lies in how this API is used by existing code: the caller may provide arbitrary integers to functions like `WMInsertInArray`. When those values are queried by C code, they may be cast back to integers or dereferenced. The knowledge of whether the value is a real address with proper provenance is only in the programmer's head. As you have suggested, I'd have to track something at runtime or split this into two different array types (one for `usize`s, one for `* mut c_void`s). I do hope to be rid of `WMArray` sooner or later, so for now I think I'll just keep the casts as-is.

trurl marked this conversation as resolved

trurl force-pushed refactor/riir from b39e16d6b2 to e0fc92bf51

2025-12-08 13:42:48 -05:00

Compare

cross reviewed 2025-12-12 08:45:42 -05:00

wutil-rs/src/array.rs

						
				@@ -2,3 +2,3 @@

				pub struct Array {

				    items: Vec<NonNull<c_void>>,

				    items: Vec<usize>,

cross commented

2025-12-12 08:45:42 -05:00

I had thought at one point that it might be nice to try to make this generic over some type T, and bifurcate usage between c_void and usize, but looking at the sheer volume of places this code is used, I think that would be tedious and premature. It's a bummer that we have to do a lot of pointer casting as a result, but ... c'est la vie.

I had thought at one point that it might be nice to try to make this generic over some type T, and bifurcate usage between `c_void` and `usize`, but looking at the sheer volume of places this code is used, I think that would be tedious and premature. It's a bummer that we have to do a lot of pointer casting as a result, but ... c'est la vie.

trurl commented

2025-12-12 16:58:58 -05:00

On the bright side, ditching WMArray and WMHashTable will be possible soon-ish.

On the bright side, ditching `WMArray` and `WMHashTable` will be possible soon-ish.

trurl marked this conversation as resolved

cross approved these changes 2025-12-12 08:45:46 -05:00

trurl merged commit e0fc92bf51 into refactor/riir

2025-12-15 12:17:23 -05:00

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: vitrine/wmaker#10

Allow edits from maintainers