[USMP] add missing const specifier for global_const_workspace

[PhilippvK]

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

More context

This bug affects the default_lib0.c file generated when using CRT AoT & USMP. See this shortened example:

Before fix:

__attribute__((section(".rodata.tvm"), ))
static struct global_const_workspace {
  float fused_constant_1_let[256] __attribute__((aligned(16))); // 1024 bytes, aligned offset: 0
  ...
} global_const_workspace = {
  .fused_constant_1_let = {
    0x1.05a71cp-3, 0x1.660c26p-2, 0x1.9765b6p-2, 0x1.dc7fdp-5, 0x1.4f3b88p-4, 0x1.1bdb82p-2, 0x1.f2443cp-3, 0x1.d3f5e4p-3,
    ...
  },
};// of total size 1284 bytes

After fix:

__attribute__((section(".rodata.tvm"), ))
static const struct global_const_workspace {
  float fused_constant_1_let[256] __attribute__((aligned(16))); // 1024 bytes, aligned offset: 0
  ...
} global_const_workspace = {
  .fused_constant_1_let = {
    0x1.05a71cp-3, 0x1.660c26p-2, 0x1.9765b6p-2, 0x1.dc7fdp-5, 0x1.4f3b88p-4, 0x1.1bdb82p-2, 0x1.f2443cp-3, 0x1.d3f5e4p-3,
    ...
  },
};// of total size 1284 bytes

Example on Compiler Explorer: https://godbolt.org/z/vrs8EnnWf

cc @Lunderberg

[Lunderberg]

Thank you for the fix, and that definitely sounds like a good fix to have. What effect did the non-const .rodata have, and can we also add a test case for it?

My naive guess would have been that the OS-level protections against writing to the .rodata section would have still been present, but language-level const checks wouldn't have been, and that any attempted writes to the .rodata fields would have resulted in runtime errors. Was that the case, and if so can we assert that a compile-time error is raised for these attempted writes instead?

[PhilippvK]

What effect did the non-const .rodata have, and can we also add a test case for it?

I got the following linker warnings when building using a recent RISC-V Gnu toolchain:

ld: warning: ../bin/prog has a LOAD segment with RWX permissions

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.

Was that the case, and if so can we assert that a compile-time error is raised for these attempted writes instead?

I have no idea if turning this specific linker-warning into an error is feasible. I guess --fatal-warnings would catch it, but we would threat any warning as an error (like gcc -Wall) which is probably to much.

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:

/tmp/cc3NSE9o.s: Assembler messages:
/tmp/cc3NSE9o.s:27: Warning: setting incorrect section attributes for .rodata.tvm

[Lunderberg]

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!

[PhilippvK]

@Lunderberg CI passed. Can we merge this?

[Lunderberg]

Can do, and thank you on the ping!