On Mon, Aug 20, 2018 at 02:54:09PM +0200, Eugeniu Rosca wrote:
Hi Tom,
On Sun, Aug 19, 2018 at 09:51:32PM -0400, Tom Rini wrote:
On Mon, Aug 20, 2018 at 02:00:25AM +0200, Eugeniu Rosca wrote:
[..]
To enable UBSAN, two prerequisites must be met from Kconfig perspective:
- ARCH has to select CONFIG_ARCH_HAS_UBSAN_SANITIZE_ALL
- defconfig has to enable CONFIG_UBSAN
This commit selects CONFIG_ARCH_HAS_UBSAN_SANITIZE_ALL for SANDBOX and ARM64 (r8a7795_salvator-x_defconfig is the only tested ARM64 platform). No defconfig changes are expected, since UBSAN is a development (not production) option. With CONFIG_UBSAN disabled, no functional change is expected from this commit.
The size increase of sanbox U-Boot (gcc 8.1.0): $ size u-boot.sandbox.* text data bss dec hex filename 1234958 80048 291472 1606478 18834e u-boot.sandbox.default 1422710 272240 291472 1986422 1e4f76 u-boot.sandbox.ubsan +187752 +192192 0 +379944
The size increase of H3 Salvator-X U-Boot (aarch64-linux-gnu-gcc 7.2.1): $ size u-boot.r8a7795-salvator-x.* text data bss dec hex filename 589954 23504 263984 877442 d6382 u-boot.r8a7795-salvator-x.default 810968 103304 263984 1178256 11fa90 u-boot.r8a7795-salvator-x.ubsan +221014 +79800 0 +300814
Can we re-work this so that there isn't a size increase unless UBSAN is enabled? I ask since I think for a v2 we should be able to say more broadly that just about everyone can enable this, but only out of the box sandbox should.
Sorry for the confusion. This commit alone does not contribute with any U-Boot binary size increase. The numbers provided above assume enabling and disabling CONFIG_UBSAN by hand via menuconfig/defconfig. I could relocate the numbers to a separate patch updating the sandbox_defconfig, if UBSAN is wanted there by default. That said, I think the contents of this commit already complies with your particular request stated above.
Ah, ok. I think it should be a bit split up into introducing framework and then enabling on sandbox, and that's where we show the size increase. But I do also think we should be able to enable the framework on most targets.