Message ID | 20210307042538.21229-15-marek.behun@nic.cz |
---|---|
State | Superseded |
Delegated to: | Tom Rini |
Headers | show |
Series | U-Boot LTO (Sandbox + Some ARM boards) | expand |
On 3/7/21 5:25 AM, Marek Behún wrote: > When compiling with LTO, the compiler fails with an error saying that > `crc_table` causes a section type conflict with `efi_var_buf`. > > This is because both are declared to be in the same section (via macro > `__efi_runtime_data`), but one is const while the other is not. > > Make this variable non-const in order to fix this. This does not look right, the crc32 array is constant. Maybe what you want to do instead if create some __efi_constant_data section ? [...] > diff --git a/lib/crc32.c b/lib/crc32.c > index e9be3bf386..c033449cff 100644 > --- a/lib/crc32.c > +++ b/lib/crc32.c > @@ -88,7 +88,7 @@ static void __efi_runtime make_crc_table(void) > * Table of CRC-32's of all single-byte values (made by make_crc_table) > */ > > -static const uint32_t __efi_runtime_data crc_table[256] = { > +static uint32_t __efi_runtime_data crc_table[256] = { > tole(0x00000000L), tole(0x77073096L), tole(0xee0e612cL), tole(0x990951baL), > tole(0x076dc419L), tole(0x706af48fL), tole(0xe963a535L), tole(0x9e6495a3L), > tole(0x0edb8832L), tole(0x79dcb8a4L), tole(0xe0d5e91eL), tole(0x97d2d988L), >
On Sun, 7 Mar 2021 05:46:24 +0100 Marek Vasut <marex@denx.de> wrote: > On 3/7/21 5:25 AM, Marek Behún wrote: > > When compiling with LTO, the compiler fails with an error saying that > > `crc_table` causes a section type conflict with `efi_var_buf`. > > > > This is because both are declared to be in the same section (via macro > > `__efi_runtime_data`), but one is const while the other is not. > > > > Make this variable non-const in order to fix this. > > This does not look right, the crc32 array is constant. > Maybe what you want to do instead if create some __efi_constant_data > section ? Yes, this was the easier solution, and maybe is not ideal. I thought it would not be much of a problem since this array can be nonconstant (generated after boot) if CONFIG_DYNAMIC_CRC_TABLE is enabled. Anyway I don't much understand the EFI code so I wanted to poke into it as little as possible. Marek
On 3/7/21 5:58 AM, Marek Behun wrote: > On Sun, 7 Mar 2021 05:46:24 +0100 > Marek Vasut <marex@denx.de> wrote: > >> On 3/7/21 5:25 AM, Marek Behún wrote: >>> When compiling with LTO, the compiler fails with an error saying that >>> `crc_table` causes a section type conflict with `efi_var_buf`. >>> >>> This is because both are declared to be in the same section (via macro >>> `__efi_runtime_data`), but one is const while the other is not. >>> >>> Make this variable non-const in order to fix this. >> >> This does not look right, the crc32 array is constant. >> Maybe what you want to do instead if create some __efi_constant_data >> section ? > > Yes, this was the easier solution, and maybe is not ideal. > > I thought it would not be much of a problem since this array can be > nonconstant (generated after boot) if CONFIG_DYNAMIC_CRC_TABLE is > enabled. > > Anyway I don't much understand the EFI code so I wanted to poke into it > as little as possible. Isn't the compiler capable of better optimization on constant stuff ? That's pretty much what prompted my suggestion to add separate section.
On Sun, 7 Mar 2021 06:02:16 +0100 Marek Vasut <marex@denx.de> wrote: > On 3/7/21 5:58 AM, Marek Behun wrote: > > On Sun, 7 Mar 2021 05:46:24 +0100 > > Marek Vasut <marex@denx.de> wrote: > > > >> On 3/7/21 5:25 AM, Marek Behún wrote: > >>> When compiling with LTO, the compiler fails with an error saying that > >>> `crc_table` causes a section type conflict with `efi_var_buf`. > >>> > >>> This is because both are declared to be in the same section (via macro > >>> `__efi_runtime_data`), but one is const while the other is not. > >>> > >>> Make this variable non-const in order to fix this. > >> > >> This does not look right, the crc32 array is constant. > >> Maybe what you want to do instead if create some __efi_constant_data > >> section ? > > > > Yes, this was the easier solution, and maybe is not ideal. > > > > I thought it would not be much of a problem since this array can be > > nonconstant (generated after boot) if CONFIG_DYNAMIC_CRC_TABLE is > > enabled. > > > > Anyway I don't much understand the EFI code so I wanted to poke into it > > as little as possible. > > Isn't the compiler capable of better optimization on constant stuff ? > That's pretty much what prompted my suggestion to add separate section. Yes, but - for this case I don't think the compiler actually can do any significat optimizations when declaring the table const, since it has to access tab[(crc ^ (x)) & 255] I tried with arm compiler just now, with -O2 and -Os, and it generated the same code either way - when using LTO, the compiler theoretically should be able to deduce that the table is never written to But if people insist on declaring the table const, I will look into this...
On Sunday 07 March 2021 13:26:36 Marek Behun wrote: > On Sun, 7 Mar 2021 06:02:16 +0100 > Marek Vasut <marex@denx.de> wrote: > > > On 3/7/21 5:58 AM, Marek Behun wrote: > > > On Sun, 7 Mar 2021 05:46:24 +0100 > > > Marek Vasut <marex@denx.de> wrote: > > > > > >> On 3/7/21 5:25 AM, Marek Behún wrote: > > >>> When compiling with LTO, the compiler fails with an error saying that > > >>> `crc_table` causes a section type conflict with `efi_var_buf`. > > >>> > > >>> This is because both are declared to be in the same section (via macro > > >>> `__efi_runtime_data`), but one is const while the other is not. > > >>> > > >>> Make this variable non-const in order to fix this. > > >> > > >> This does not look right, the crc32 array is constant. > > >> Maybe what you want to do instead if create some __efi_constant_data > > >> section ? > > > > > > Yes, this was the easier solution, and maybe is not ideal. > > > > > > I thought it would not be much of a problem since this array can be > > > nonconstant (generated after boot) if CONFIG_DYNAMIC_CRC_TABLE is > > > enabled. > > > > > > Anyway I don't much understand the EFI code so I wanted to poke into it > > > as little as possible. > > > > Isn't the compiler capable of better optimization on constant stuff ? > > That's pretty much what prompted my suggestion to add separate section. > > Yes, but > - for this case I don't think the compiler actually can do any > significat optimizations when declaring the table const, since it has > to access > tab[(crc ^ (x)) & 255] > I tried with arm compiler just now, with -O2 and -Os, and it > generated the same code either way > - when using LTO, the compiler theoretically should be able to deduce > that the table is never written to > > But if people insist on declaring the table const, I will look into > this... If you try to overwrite a const variable from the same code unit then compiler throw an error. So declaring read-only variable as a const could prevent people to unintentionally overwrite read-only variable. And detect possible bad code.
On Sun, 7 Mar 2021 13:31:11 +0100 Pali Rohár <pali@kernel.org> wrote: > On Sunday 07 March 2021 13:26:36 Marek Behun wrote: > > On Sun, 7 Mar 2021 06:02:16 +0100 > > Marek Vasut <marex@denx.de> wrote: > > > > > On 3/7/21 5:58 AM, Marek Behun wrote: > > > > On Sun, 7 Mar 2021 05:46:24 +0100 > > > > Marek Vasut <marex@denx.de> wrote: > > > > > > > >> On 3/7/21 5:25 AM, Marek Behún wrote: > > > >>> When compiling with LTO, the compiler fails with an error saying that > > > >>> `crc_table` causes a section type conflict with `efi_var_buf`. > > > >>> > > > >>> This is because both are declared to be in the same section (via macro > > > >>> `__efi_runtime_data`), but one is const while the other is not. > > > >>> > > > >>> Make this variable non-const in order to fix this. > > > >> > > > >> This does not look right, the crc32 array is constant. > > > >> Maybe what you want to do instead if create some __efi_constant_data > > > >> section ? > > > > > > > > Yes, this was the easier solution, and maybe is not ideal. > > > > > > > > I thought it would not be much of a problem since this array can be > > > > nonconstant (generated after boot) if CONFIG_DYNAMIC_CRC_TABLE is > > > > enabled. > > > > > > > > Anyway I don't much understand the EFI code so I wanted to poke into it > > > > as little as possible. > > > > > > Isn't the compiler capable of better optimization on constant stuff ? > > > That's pretty much what prompted my suggestion to add separate section. > > > > Yes, but > > - for this case I don't think the compiler actually can do any > > significat optimizations when declaring the table const, since it has > > to access > > tab[(crc ^ (x)) & 255] > > I tried with arm compiler just now, with -O2 and -Os, and it > > generated the same code either way > > - when using LTO, the compiler theoretically should be able to deduce > > that the table is never written to > > > > But if people insist on declaring the table const, I will look into > > this... > > If you try to overwrite a const variable from the same code unit then > compiler throw an error. So declaring read-only variable as a const > could prevent people to unintentionally overwrite read-only variable. > And detect possible bad code. OK it seems all linker scripts also mention .rodata.efi_runtime*, not just .data.efi_runtime*. I shall put it into the .rodata.efi_runtime section. Marek
diff --git a/lib/crc32.c b/lib/crc32.c index e9be3bf386..c033449cff 100644 --- a/lib/crc32.c +++ b/lib/crc32.c @@ -88,7 +88,7 @@ static void __efi_runtime make_crc_table(void) * Table of CRC-32's of all single-byte values (made by make_crc_table) */ -static const uint32_t __efi_runtime_data crc_table[256] = { +static uint32_t __efi_runtime_data crc_table[256] = { tole(0x00000000L), tole(0x77073096L), tole(0xee0e612cL), tole(0x990951baL), tole(0x076dc419L), tole(0x706af48fL), tole(0xe963a535L), tole(0x9e6495a3L), tole(0x0edb8832L), tole(0x79dcb8a4L), tole(0xe0d5e91eL), tole(0x97d2d988L),
When compiling with LTO, the compiler fails with an error saying that `crc_table` causes a section type conflict with `efi_var_buf`. This is because both are declared to be in the same section (via macro `__efi_runtime_data`), but one is const while the other is not. Make this variable non-const in order to fix this. Signed-off-by: Marek Behún <marek.behun@nic.cz> --- lib/crc32.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)