-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
[USMP] add missing const specifier for global_const_workspace #16999
Conversation
The `.rodata*` section of any program should not be writable. The missing `const` specifier in `static struct global_const_workspace {...}` leads to the following `readelf -e` output (shortened): ``` Section Headers: [Nr] Name Type Addr Off Size ES Flg Lk Inf Al [ 0] NULL 00000000 000000 000000 00 0 0 0 [ 1] .text PROGBITS 00000000 001000 009fbe 00 AX 0 0 16 [ 2] .rodata PROGBITS 00009fc0 00afc0 000e50 00 WA 0 0 16 [ 3] .srodata PROGBITS 0000ae10 00be10 000068 08 AM 0 0 8 ... ``` After this fix, the output looks as follows (`AW` -> `A`): ``` Section Headers: [Nr] Name Type Addr Off Size ES Flg Lk Inf Al [ 0] NULL 00000000 000000 000000 00 0 0 0 [ 1] .text PROGBITS 00000000 001000 00a1be 00 AX 0 0 16 [ 2] .rodata PROGBITS 0000a1c0 00b1c0 000e50 00 A 0 0 16 [ 3] .srodata PROGBITS 0000b010 00c010 000070 00 A 0 0 8 ```
Thank you for the fix, and that definitely sounds like a good fix to have. What effect did the non-const My naive guess would have been that the OS-level protections against writing to the |
I got the following linker warnings when building using a recent RISC-V Gnu toolchain:
I first expected this to be a bug in the linker script but found the TVM bug when trying to solve it. At runtime (baremetal-level instruction set simulation) this had no impact. However I did not tried it on other architectures.
I have no idea if turning this specific linker-warning into an error is feasible. I guess I will try test this also on other targets with other target devices to find out if I can trigger any runtime errors caused by this bug. Update: With other targets, I got a similar warning even before linkage:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense, both on the warning encountered and the difficulty of making a targeted test case for it. I suppose in principle we could capture stdout/stderr for a test compilation and assert that both are empty, but that would still be a very compiler-dependent test.
Approved!
@Lunderberg CI passed. Can we merge this? |
Can do, and thank you on the ping! |
The
.rodata*
section of any program should not be writable.The missing
const
specifier instatic struct global_const_workspace {...}
leads to the followingreadelf -e
output (shortened):After this fix, the output looks as follows (
AW -> A
):More context
This bug affects the
default_lib0.c
file generated when using CRT AoT & USMP. See this shortened example:Before fix:
After fix:
Example on Compiler Explorer: https://godbolt.org/z/vrs8EnnWf
cc @Lunderberg