Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42794 )
Change subject: crossgcc: Allow GCC to get asan shadow offset at runtime ......................................................................
Patch Set 16:
Patch Set 16:
Patch Set 16:
Patch Set 13:
Patch Set 13:
For the CAR systems, can't you add the shadow region at CONFIG_DCACHE_RAM_BASE? For ramstage does the compiler not add relocations for the shadow region?
What would be the benefit of having shadow region located at CONFIG_DCACHE_RAM_BASE? Do you have in mind to skip this gcc patch in order to have a common location over multiple platforms?
Exactly. GCC only accepts a constant address, so you could use CONFIG_DCACHE_RAM_BASE and avoid needing the patch. Unless I'm missing something.
The benefits the gcc patch provides are:
- We can place the shadow region in a separate linker section with all its advantages like automatic fit insurance (the linker makes sure that in the end the memory layout is not violated...even years after this feature was developed and coreboot tree has evolved a lot).
- We do not modify the memory layout compared to the current one (as we are placing the shadow region at the end of CAR).
- We can be much more flexible if needed (thinking of other stages like even bootblock).
To me, these benefits justifies this relatively small gcc patch. And we have made sure that if you compile your tree with ASan enabled but have missed the patch, it will end up in a compilation error. So you cannot accidentally enable the feature and have your gcc unpatched.
Do you have a strong objection against this gcc patch? And if yes, for what reason?
No strong objection. Was just wondering if it was really necessary. I the commit message should be updated to include some of the reasoning.
-
Raul Rangel (Code Review)