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

Effectiveness of F2, Control-X and so on #18

Closed
grahamperrin opened this issue Sep 22, 2019 · 5 comments
Closed

Effectiveness of F2, Control-X and so on #18

grahamperrin opened this issue Sep 22, 2019 · 5 comments

Comments

@grahamperrin
Copy link

Re: https://github.com/adsr/mle#user-content-basic-usage and #17 (comment) there's no response to F2 or Control-X on FreeBSD.

Please, what am I missing?

Should I report a bug in the FreeBSD area? https://www.freshports.org/editors/mle/

root@momh167-gjp4-8570p:~ # date ;  uname -v
Sun Sep 22 13:28:49 BST 2019
FreeBSD 13.0-CURRENT r352535 GENERIC 
root@momh167-gjp4-8570p:~ # pkg query '%o %v %R' mle
editors/mle 1.4.1 FreeBSD
root@momh167-gjp4-8570p:~ # 
@adsr
Copy link
Owner

adsr commented Sep 22, 2019

I see the same behavior when installing the port but not when compiling from source. Digging.

@adsr
Copy link
Owner

adsr commented Sep 22, 2019

Thanks for the report @grahamperrin. I think 31e89a6 will fix this issue. I'll update the port after fix is confirmed.

To explain what is going on in these last few commits... The editor stores key bindings in a hash map. This hash map is keyed by a struct (kinput_t) which contain 3 integer fields (u8, u32, u16), however most systems add padding bytes so that the actual struct in memory looks like (u8, padding24, u32, u16, padding16) totaling 12 bytes. The entire struct is hashed when keying into the map, so the padding bytes matter. The problem is that assigning a kinput_t to a struct literal like input = (kinput_t){..., ..., ...}; leaves the padding bytes undefined.

Surprisingly this bug only surfaced on certain build configurations on FreeBSD. I cannot fully explain why this only affects certain build configurations, nor why the last commit (af0444b) fixed the previously failing functional test but broke all user input for the prod build. In gdb the faulty padding bytes were always 0xff which was part of the last fix, so I presume the compiler made an unlucky guess when sharing the kinput_t stack variable.

Commit 31e89a6 is more vigilant about how kinput_t values are set and passed around (memset and memcpy are used). A unit test for the padding byte issue is included. Another possible fix for this is to define kinput_t as a packed struct, however I'm not sure how portable that is or what kind of guarantees are made concerning padding bytes on various systems.

@adsr
Copy link
Owner

adsr commented Sep 25, 2019

@adsr
Copy link
Owner

adsr commented Sep 25, 2019

Should be fixed now. Thanks again for the report.

@adsr adsr closed this as completed Sep 25, 2019
@grahamperrin
Copy link
Author

Fix confirmed – thanks.

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

No branches or pull requests

2 participants