| Message ID | 20180820185431.57495-2-sjg@chromium.org |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [U-Boot,RESEND,v9,01/18] Revert "efi_loader: Rename sections to allow for implicit data" | expand |
On 20.08.18 20:54, Simon Glass wrote: > This partially reverts commit 7e21fbca26d18327cf7cabaad08df276a06a07d8. > > That change broke sandbox EFI support for unknown reasons. It also changes Wouldn't it be better to just figure out the reasons? So far all bugs I've found were linker script related and quite obvious once you start to dig into them. > sandbox to use--gc-sections which we don't want. Why don't we want gc-sections with sandbox? > For now I am just reverting the sandbox portion as presumably this change > is safe on other architectures. Sandbox is your target, so you're free to do whatever you like :). But I'm not sure this is the right path forward. I'd rather like to keep things consistent. So what do you expect happens with this patch? A resend of a patch 1/18 by itself doesn't really tell me what you're trying to say. Alex
Hi Alex, On 20 August 2018 at 16:29, Alexander Graf <agraf@suse.de> wrote: > > > > On 20.08.18 20:54, Simon Glass wrote: > > This partially reverts commit 7e21fbca26d18327cf7cabaad08df276a06a07d8. > > > > That change broke sandbox EFI support for unknown reasons. It also changes > > Wouldn't it be better to just figure out the reasons? So far all bugs > I've found were linker script related and quite obvious once you start > to dig into them. > > > sandbox to use--gc-sections which we don't want. > > Why don't we want gc-sections with sandbox? It is a space optimisation which we don't need for sandbox. It also complicates the object files unnecessarily. Put another way, why is it desirable? > > > For now I am just reverting the sandbox portion as presumably this change > > is safe on other architectures. > > Sandbox is your target, so you're free to do whatever you like :). But > I'm not sure this is the right path forward. I'd rather like to keep > things consistent. In what sense? > > So what do you expect happens with this patch? A resend of a patch 1/18 > by itself doesn't really tell me what you're trying to say. The resend was due to me noticing that people did not get the patch on cc. I only sent this one patch, but I can resend send the whole series if you like. Regards, Simon
On 21.08.18 19:31, Simon Glass wrote: > Hi Alex, > > On 20 August 2018 at 16:29, Alexander Graf <agraf@suse.de> wrote: >> >> >> >> On 20.08.18 20:54, Simon Glass wrote: >>> This partially reverts commit 7e21fbca26d18327cf7cabaad08df276a06a07d8. >>> >>> That change broke sandbox EFI support for unknown reasons. It also changes >> >> Wouldn't it be better to just figure out the reasons? So far all bugs >> I've found were linker script related and quite obvious once you start >> to dig into them. >> >>> sandbox to use--gc-sections which we don't want. >> >> Why don't we want gc-sections with sandbox? > > It is a space optimisation which we don't need for sandbox. It also > complicates the object files unnecessarily. > > Put another way, why is it desirable? The reason we need it for efi_loader is that runtime section code may generate implicit dependencies on "other data" such as strings, jump tables, etc that are generated on the fly by the compiler. The only way to ensure that all these implicit pieces of data are actually in the runtime section is by forcing function and data sections onto gcc, because then the implicit pieces of data will inherit the name tags of the function they're in. > >> >>> For now I am just reverting the sandbox portion as presumably this change >>> is safe on other architectures. >> >> Sandbox is your target, so you're free to do whatever you like :). But >> I'm not sure this is the right path forward. I'd rather like to keep >> things consistent. > > In what sense? For efi_loader Runtime Services to actually work we *have* to have -fdata-sections and -ffunction-sections enabled, otherwise they just break. So by not enabling them with sandbox we don't get runtime service support after ExitBootServices (which is useless for sandbox anyway really), but most importantly we're different from all other targets. > >> >> So what do you expect happens with this patch? A resend of a patch 1/18 >> by itself doesn't really tell me what you're trying to say. > > The resend was due to me noticing that people did not get the patch on > cc. I only sent this one patch, but I can resend send the whole series > if you like. I think that it makes sense to differentiate between "this should be applied for *this* release" and "this is a patch set that as a whole should be applied". Or in other words: If the sections really do break sandbox (and again, for me they don't - I am unable to reproduce still), I would expect that you send that patch individually as a resend :). As it stood right now, I simply didn't grasp what your intention was so I didn't include it in my pull request. Alex
Hi Alex, On 21 August 2018 at 13:24, Alexander Graf <agraf@suse.de> wrote: > > > On 21.08.18 19:31, Simon Glass wrote: >> Hi Alex, >> >> On 20 August 2018 at 16:29, Alexander Graf <agraf@suse.de> wrote: >>> >>> >>> >>> On 20.08.18 20:54, Simon Glass wrote: >>>> This partially reverts commit 7e21fbca26d18327cf7cabaad08df276a06a07d8. >>>> >>>> That change broke sandbox EFI support for unknown reasons. It also changes >>> >>> Wouldn't it be better to just figure out the reasons? So far all bugs >>> I've found were linker script related and quite obvious once you start >>> to dig into them. >>> >>>> sandbox to use--gc-sections which we don't want. >>> >>> Why don't we want gc-sections with sandbox? >> >> It is a space optimisation which we don't need for sandbox. It also >> complicates the object files unnecessarily. >> >> Put another way, why is it desirable? > > The reason we need it for efi_loader is that runtime section code may > generate implicit dependencies on "other data" such as strings, jump > tables, etc that are generated on the fly by the compiler. > > The only way to ensure that all these implicit pieces of data are > actually in the runtime section is by forcing function and data sections > onto gcc, because then the implicit pieces of data will inherit the name > tags of the function they're in. Do they actually not end up in the runtime section without -fdata-sections, etc.? I would expect that this would just increase the size of the runtime section (potentially quite a lot). It seems wrong for them to be omitted by the toolchain. Or are you saying that they get picked up by an earlier .lds line, and so are not available for the (later) runtime section line? > >> >>> >>>> For now I am just reverting the sandbox portion as presumably this change >>>> is safe on other architectures. >>> >>> Sandbox is your target, so you're free to do whatever you like :). But >>> I'm not sure this is the right path forward. I'd rather like to keep >>> things consistent. >> >> In what sense? > > For efi_loader Runtime Services to actually work we *have* to have > -fdata-sections and -ffunction-sections enabled, otherwise they just > break. So by not enabling them with sandbox we don't get runtime service > support after ExitBootServices (which is useless for sandbox anyway > really), but most importantly we're different from all other targets. I'm happy to be different from other targets for now. As you say, we cannot support run-time services for sandbox, at least without some cunning plan I am not aware of. > >> >>> >>> So what do you expect happens with this patch? A resend of a patch 1/18 >>> by itself doesn't really tell me what you're trying to say. >> >> The resend was due to me noticing that people did not get the patch on >> cc. I only sent this one patch, but I can resend send the whole series >> if you like. > > I think that it makes sense to differentiate between "this should be > applied for *this* release" and "this is a patch set that as a whole > should be applied". > > Or in other words: If the sections really do break sandbox (and again, > for me they don't - I am unable to reproduce still), I would expect that > you send that patch individually as a resend :). > > As it stood right now, I simply didn't grasp what your intention was so > I didn't include it in my pull request. Well I think the revert is needed for this release. For the rest of the patches, perhaps they should be reviewed and applied when convenient? You should be able to repeat the breakage by using my series without the review (u-boot-dm/efi-working). Regards, Simon
On 08/23/2018 12:45 PM, Simon Glass wrote: > Hi Alex, > > On 21 August 2018 at 13:24, Alexander Graf <agraf@suse.de> wrote: >> >> On 21.08.18 19:31, Simon Glass wrote: >>> Hi Alex, >>> >>> On 20 August 2018 at 16:29, Alexander Graf <agraf@suse.de> wrote: >>>> >>>> >>>> On 20.08.18 20:54, Simon Glass wrote: >>>>> This partially reverts commit 7e21fbca26d18327cf7cabaad08df276a06a07d8. >>>>> >>>>> That change broke sandbox EFI support for unknown reasons. It also changes >>>> Wouldn't it be better to just figure out the reasons? So far all bugs >>>> I've found were linker script related and quite obvious once you start >>>> to dig into them. >>>> >>>>> sandbox to use--gc-sections which we don't want. >>>> Why don't we want gc-sections with sandbox? >>> It is a space optimisation which we don't need for sandbox. It also >>> complicates the object files unnecessarily. >>> >>> Put another way, why is it desirable? >> The reason we need it for efi_loader is that runtime section code may >> generate implicit dependencies on "other data" such as strings, jump >> tables, etc that are generated on the fly by the compiler. >> >> The only way to ensure that all these implicit pieces of data are >> actually in the runtime section is by forcing function and data sections >> onto gcc, because then the implicit pieces of data will inherit the name >> tags of the function they're in. > Do they actually not end up in the runtime section without > -fdata-sections, etc.? I would expect that this would just increase Yes, that's exactly what happens ;). Without data and function sections, implicit pieces of information go into the big bucket sections (.data/.rodata/.bss) even when the function they're used in is in an explicit section. > the size of the runtime section (potentially quite a lot). It seems > wrong for them to be omitted by the toolchain. > > Or are you saying that they get picked up by an earlier .lds line, and > so are not available for the (later) runtime section line? Exactly. Because they're in the generic sections, there is no way to determine that these variables really are runtime relevant at link time. > >>>>> For now I am just reverting the sandbox portion as presumably this change >>>>> is safe on other architectures. >>>> Sandbox is your target, so you're free to do whatever you like :). But >>>> I'm not sure this is the right path forward. I'd rather like to keep >>>> things consistent. >>> In what sense? >> For efi_loader Runtime Services to actually work we *have* to have >> -fdata-sections and -ffunction-sections enabled, otherwise they just >> break. So by not enabling them with sandbox we don't get runtime service >> support after ExitBootServices (which is useless for sandbox anyway >> really), but most importantly we're different from all other targets. > I'm happy to be different from other targets for now. As you say, we > cannot support run-time services for sandbox, at least without some > cunning plan I am not aware of. > >>>> So what do you expect happens with this patch? A resend of a patch 1/18 >>>> by itself doesn't really tell me what you're trying to say. >>> The resend was due to me noticing that people did not get the patch on >>> cc. I only sent this one patch, but I can resend send the whole series >>> if you like. >> I think that it makes sense to differentiate between "this should be >> applied for *this* release" and "this is a patch set that as a whole >> should be applied". >> >> Or in other words: If the sections really do break sandbox (and again, >> for me they don't - I am unable to reproduce still), I would expect that >> you send that patch individually as a resend :). >> >> As it stood right now, I simply didn't grasp what your intention was so >> I didn't include it in my pull request. > Well I think the revert is needed for this release. For the rest of > the patches, perhaps they should be reviewed and applied when > convenient? > > You should be able to repeat the breakage by using my series without > the review (u-boot-dm/efi-working). Do you see any breakage with current master? Alex
Hi Alex, On 23 August 2018 at 06:21, Alexander Graf <agraf@suse.de> wrote: > > On 08/23/2018 12:45 PM, Simon Glass wrote: >> >> Hi Alex, >> >> On 21 August 2018 at 13:24, Alexander Graf <agraf@suse.de> wrote: >>> >>> >>> On 21.08.18 19:31, Simon Glass wrote: >>>> >>>> Hi Alex, >>>> >>>> On 20 August 2018 at 16:29, Alexander Graf <agraf@suse.de> wrote: >>>>> >>>>> >>>>> >>>>> On 20.08.18 20:54, Simon Glass wrote: >>>>>> >>>>>> This partially reverts commit 7e21fbca26d18327cf7cabaad08df276a06a07d8. >>>>>> >>>>>> That change broke sandbox EFI support for unknown reasons. It also changes >>>>> >>>>> Wouldn't it be better to just figure out the reasons? So far all bugs >>>>> I've found were linker script related and quite obvious once you start >>>>> to dig into them. >>>>> >>>>>> sandbox to use--gc-sections which we don't want. >>>>> >>>>> Why don't we want gc-sections with sandbox? >>>> >>>> It is a space optimisation which we don't need for sandbox. It also >>>> complicates the object files unnecessarily. >>>> >>>> Put another way, why is it desirable? >>> >>> The reason we need it for efi_loader is that runtime section code may >>> generate implicit dependencies on "other data" such as strings, jump >>> tables, etc that are generated on the fly by the compiler. >>> >>> The only way to ensure that all these implicit pieces of data are >>> actually in the runtime section is by forcing function and data sections >>> onto gcc, because then the implicit pieces of data will inherit the name >>> tags of the function they're in. >> >> Do they actually not end up in the runtime section without >> -fdata-sections, etc.? I would expect that this would just increase > > > Yes, that's exactly what happens ;). Without data and function sections, implicit pieces of information go into the big bucket sections (.data/.rodata/.bss) even when the function they're used in is in an explicit section. > >> the size of the runtime section (potentially quite a lot). It seems >> wrong for them to be omitted by the toolchain. >> >> Or are you saying that they get picked up by an earlier .lds line, and >> so are not available for the (later) runtime section line? > > > Exactly. Because they're in the generic sections, there is no way to determine that these variables really are runtime relevant at link time. > > >> >>>>>> For now I am just reverting the sandbox portion as presumably this change >>>>>> is safe on other architectures. >>>>> >>>>> Sandbox is your target, so you're free to do whatever you like :). But >>>>> I'm not sure this is the right path forward. I'd rather like to keep >>>>> things consistent. >>>> >>>> In what sense? >>> >>> For efi_loader Runtime Services to actually work we *have* to have >>> -fdata-sections and -ffunction-sections enabled, otherwise they just >>> break. So by not enabling them with sandbox we don't get runtime service >>> support after ExitBootServices (which is useless for sandbox anyway >>> really), but most importantly we're different from all other targets. >> >> I'm happy to be different from other targets for now. As you say, we >> cannot support run-time services for sandbox, at least without some >> cunning plan I am not aware of. >> >>>>> So what do you expect happens with this patch? A resend of a patch 1/18 >>>>> by itself doesn't really tell me what you're trying to say. >>>> >>>> The resend was due to me noticing that people did not get the patch on >>>> cc. I only sent this one patch, but I can resend send the whole series >>>> if you like. >>> >>> I think that it makes sense to differentiate between "this should be >>> applied for *this* release" and "this is a patch set that as a whole >>> should be applied". >>> >>> Or in other words: If the sections really do break sandbox (and again, >>> for me they don't - I am unable to reproduce still), I would expect that >>> you send that patch individually as a resend :). >>> >>> As it stood right now, I simply didn't grasp what your intention was so >>> I didn't include it in my pull request. >> >> Well I think the revert is needed for this release. For the rest of >> the patches, perhaps they should be reviewed and applied when >> convenient? >> >> You should be able to repeat the breakage by using my series without >> the review (u-boot-dm/efi-working). > > > Do you see any breakage with current master? No the tests all pass. I still want a revert though. Regards, Simon
diff --git a/arch/sandbox/config.mk b/arch/sandbox/config.mk index 5e7077bfe75..2babcde8815 100644 --- a/arch/sandbox/config.mk +++ b/arch/sandbox/config.mk @@ -5,9 +5,6 @@ PLATFORM_CPPFLAGS += -D__SANDBOX__ -U_FORTIFY_SOURCE PLATFORM_CPPFLAGS += -DCONFIG_ARCH_MAP_SYSMEM PLATFORM_LIBS += -lrt -LDFLAGS_FINAL += --gc-sections -PLATFORM_RELFLAGS += -ffunction-sections -fdata-sections - # Define this to avoid linking with SDL, which requires SDL libraries # This can solve 'sdl-config: Command not found' errors ifneq ($(NO_SDL),) diff --git a/arch/sandbox/cpu/u-boot.lds b/arch/sandbox/cpu/u-boot.lds index 727bcc35981..3a6cf55eb99 100644 --- a/arch/sandbox/cpu/u-boot.lds +++ b/arch/sandbox/cpu/u-boot.lds @@ -24,9 +24,8 @@ SECTIONS } .efi_runtime : { - *(.text.efi_runtime*) - *(.rodata.efi_runtime*) - *(.data.efi_runtime*) + *(efi_runtime_text) + *(efi_runtime_data) } .__efi_runtime_stop : { @@ -39,8 +38,8 @@ SECTIONS } .efi_runtime_rel : { - *(.rel*.efi_runtime) - *(.rel*.efi_runtime.*) + *(.relefi_runtime_text) + *(.relefi_runtime_data) } .efi_runtime_rel_stop :
This partially reverts commit 7e21fbca26d18327cf7cabaad08df276a06a07d8. That change broke sandbox EFI support for unknown reasons. It also changes sandbox to use--gc-sections which we don't want. For now I am just reverting the sandbox portion as presumably this change is safe on other architectures. Fixes: 7e21fbca26 (efi_loader: Rename sections to allow for implicit data) Signed-off-by: Simon Glass <sjg@chromium.org> --- Changes in v9: - Add revert for "efi_loader: Rename sections to allow for implicit data" Changes in v8: None Changes in v7: None Changes in v6: None Changes in v5: None Changes in v4: None Changes in v3: None Changes in v2: None arch/sandbox/config.mk | 3 --- arch/sandbox/cpu/u-boot.lds | 9 ++++----- 2 files changed, 4 insertions(+), 8 deletions(-)