| Message ID | 97bebbfa-b5e8-4c57-162f-2a052de92f02@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] x86: avoid SORT_BY_INIT_PRIORITY with old GNU ld | expand |
On Fri, Mar 11, 2022 at 02:28:40PM +0100, Jan Beulich wrote: > Support for this construct was added in 2.22 only. Avoid the need to > introduce logic to probe for linker script capabilities by (ab)using the > probe for a command line option having appeared at about the same time. > > Fixes: 4b7fd8153ddf ("x86: fold sections in final binaries") > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > v2: Always define HAVE_LD_SORT_BY_INIT_PRIORITY when using LLVM ld. > > --- a/xen/arch/x86/arch.mk > +++ b/xen/arch/x86/arch.mk > @@ -73,6 +73,16 @@ ifeq ($(CONFIG_UBSAN),y) > $(call cc-option-add,CFLAGS_UBSAN,CC,-fno-sanitize=alignment) > endif > > +ifeq ($(call success,$(LD) --version | head -n 1 | grep -q "GNU ld"),y) You are not going to like this, but I think this should live in xen/Kconfig together with CC_IS_{GCC,CLANG}, ie: LD_IS_GNU or similar. It's possible we will need this in the future in other places, so having it in Kconfig makes sense. > +# While not much better than going by raw GNU ld version, utilize that the > +# feature we're after has appeared in the same release as the > +# --print-output-format command line option. > +AFLAGS-$(call ld-option,--print-output-format) += -DHAVE_LD_SORT_BY_INIT_PRIORITY That's fine to have here IMO. Thanks, Roger.
On 11.03.2022 15:34, Roger Pau Monné wrote: > On Fri, Mar 11, 2022 at 02:28:40PM +0100, Jan Beulich wrote: >> Support for this construct was added in 2.22 only. Avoid the need to >> introduce logic to probe for linker script capabilities by (ab)using the >> probe for a command line option having appeared at about the same time. >> >> Fixes: 4b7fd8153ddf ("x86: fold sections in final binaries") >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> v2: Always define HAVE_LD_SORT_BY_INIT_PRIORITY when using LLVM ld. >> >> --- a/xen/arch/x86/arch.mk >> +++ b/xen/arch/x86/arch.mk >> @@ -73,6 +73,16 @@ ifeq ($(CONFIG_UBSAN),y) >> $(call cc-option-add,CFLAGS_UBSAN,CC,-fno-sanitize=alignment) >> endif >> >> +ifeq ($(call success,$(LD) --version | head -n 1 | grep -q "GNU ld"),y) > > You are not going to like this, but I think this should live in > xen/Kconfig together with CC_IS_{GCC,CLANG}, ie: LD_IS_GNU or similar. > > It's possible we will need this in the future in other places, so > having it in Kconfig makes sense. Despite me not liking this (until, as said elsewhere, we've properly settled on either approach) I did actually consider doing like you suggest. But: I would have to introduce there more than I need here, just for consistency's sake, and I wouldn't have a way to test the LLD part of it (I did check - none of the distros where I chose to install Clang offer the linker). I realize I could ask you to help with the testing, but then the first point would remain. I'd prefer if for this simple build fix it was okay to go the old fashioned route ... Jan
On Fri, Mar 11, 2022 at 03:55:57PM +0100, Jan Beulich wrote: > On 11.03.2022 15:34, Roger Pau Monné wrote: > > On Fri, Mar 11, 2022 at 02:28:40PM +0100, Jan Beulich wrote: > >> Support for this construct was added in 2.22 only. Avoid the need to > >> introduce logic to probe for linker script capabilities by (ab)using the > >> probe for a command line option having appeared at about the same time. > >> > >> Fixes: 4b7fd8153ddf ("x86: fold sections in final binaries") > >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > >> --- > >> v2: Always define HAVE_LD_SORT_BY_INIT_PRIORITY when using LLVM ld. > >> > >> --- a/xen/arch/x86/arch.mk > >> +++ b/xen/arch/x86/arch.mk > >> @@ -73,6 +73,16 @@ ifeq ($(CONFIG_UBSAN),y) > >> $(call cc-option-add,CFLAGS_UBSAN,CC,-fno-sanitize=alignment) > >> endif > >> > >> +ifeq ($(call success,$(LD) --version | head -n 1 | grep -q "GNU ld"),y) > > > > You are not going to like this, but I think this should live in > > xen/Kconfig together with CC_IS_{GCC,CLANG}, ie: LD_IS_GNU or similar. > > > > It's possible we will need this in the future in other places, so > > having it in Kconfig makes sense. > > Despite me not liking this (until, as said elsewhere, we've properly > settled on either approach) I did actually consider doing like you > suggest. But: I would have to introduce there more than I need here, > just for consistency's sake, and I wouldn't have a way to test the > LLD part of it (I did check - none of the distros where I chose to > install Clang offer the linker). I realize I could ask you to help > with the testing, but then the first point would remain. I'd prefer > if for this simple build fix it was okay to go the old fashioned > route ... I would be fine with you just introducing LD_IS_GNU. That's all we need so far. We can introduce LD_IS_LLVM if/when required. I prefer that asymmetry rather than doing the detection here. Thanks, Roger.
On 11.03.2022 16:07, Roger Pau Monné wrote: > On Fri, Mar 11, 2022 at 03:55:57PM +0100, Jan Beulich wrote: >> On 11.03.2022 15:34, Roger Pau Monné wrote: >>> On Fri, Mar 11, 2022 at 02:28:40PM +0100, Jan Beulich wrote: >>>> Support for this construct was added in 2.22 only. Avoid the need to >>>> introduce logic to probe for linker script capabilities by (ab)using the >>>> probe for a command line option having appeared at about the same time. >>>> >>>> Fixes: 4b7fd8153ddf ("x86: fold sections in final binaries") >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>> --- >>>> v2: Always define HAVE_LD_SORT_BY_INIT_PRIORITY when using LLVM ld. >>>> >>>> --- a/xen/arch/x86/arch.mk >>>> +++ b/xen/arch/x86/arch.mk >>>> @@ -73,6 +73,16 @@ ifeq ($(CONFIG_UBSAN),y) >>>> $(call cc-option-add,CFLAGS_UBSAN,CC,-fno-sanitize=alignment) >>>> endif >>>> >>>> +ifeq ($(call success,$(LD) --version | head -n 1 | grep -q "GNU ld"),y) >>> >>> You are not going to like this, but I think this should live in >>> xen/Kconfig together with CC_IS_{GCC,CLANG}, ie: LD_IS_GNU or similar. >>> >>> It's possible we will need this in the future in other places, so >>> having it in Kconfig makes sense. >> >> Despite me not liking this (until, as said elsewhere, we've properly >> settled on either approach) I did actually consider doing like you >> suggest. But: I would have to introduce there more than I need here, >> just for consistency's sake, and I wouldn't have a way to test the >> LLD part of it (I did check - none of the distros where I chose to >> install Clang offer the linker). I realize I could ask you to help >> with the testing, but then the first point would remain. I'd prefer >> if for this simple build fix it was okay to go the old fashioned >> route ... > > I would be fine with you just introducing LD_IS_GNU. That's all we > need so far. We can introduce LD_IS_LLVM if/when required. I prefer > that asymmetry rather than doing the detection here. Yet I'm not happy to go this route. I'm only willing to do this consistently, but that in turn not without us having formally sat down and discussed the overall model to use. The only short term alternative I see is to go back to SORT() unilaterally, hoping that for now different priorities won't be encountered. Jan
On Mon, Mar 14, 2022 at 08:38:52AM +0100, Jan Beulich wrote: > On 11.03.2022 16:07, Roger Pau Monné wrote: > > On Fri, Mar 11, 2022 at 03:55:57PM +0100, Jan Beulich wrote: > >> On 11.03.2022 15:34, Roger Pau Monné wrote: > >>> On Fri, Mar 11, 2022 at 02:28:40PM +0100, Jan Beulich wrote: > >>>> Support for this construct was added in 2.22 only. Avoid the need to > >>>> introduce logic to probe for linker script capabilities by (ab)using the > >>>> probe for a command line option having appeared at about the same time. > >>>> > >>>> Fixes: 4b7fd8153ddf ("x86: fold sections in final binaries") > >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> > >>>> --- > >>>> v2: Always define HAVE_LD_SORT_BY_INIT_PRIORITY when using LLVM ld. > >>>> > >>>> --- a/xen/arch/x86/arch.mk > >>>> +++ b/xen/arch/x86/arch.mk > >>>> @@ -73,6 +73,16 @@ ifeq ($(CONFIG_UBSAN),y) > >>>> $(call cc-option-add,CFLAGS_UBSAN,CC,-fno-sanitize=alignment) > >>>> endif > >>>> > >>>> +ifeq ($(call success,$(LD) --version | head -n 1 | grep -q "GNU ld"),y) > >>> > >>> You are not going to like this, but I think this should live in > >>> xen/Kconfig together with CC_IS_{GCC,CLANG}, ie: LD_IS_GNU or similar. > >>> > >>> It's possible we will need this in the future in other places, so > >>> having it in Kconfig makes sense. > >> > >> Despite me not liking this (until, as said elsewhere, we've properly > >> settled on either approach) I did actually consider doing like you > >> suggest. But: I would have to introduce there more than I need here, > >> just for consistency's sake, and I wouldn't have a way to test the > >> LLD part of it (I did check - none of the distros where I chose to > >> install Clang offer the linker). I realize I could ask you to help > >> with the testing, but then the first point would remain. I'd prefer > >> if for this simple build fix it was okay to go the old fashioned > >> route ... > > > > I would be fine with you just introducing LD_IS_GNU. That's all we > > need so far. We can introduce LD_IS_LLVM if/when required. I prefer > > that asymmetry rather than doing the detection here. > > Yet I'm not happy to go this route. I'm only willing to do this > consistently, but that in turn not without us having formally sat down > and discussed the overall model to use. The only short term alternative > I see is to go back to SORT() unilaterally, hoping that for now > different priorities won't be encountered. Would you be fine if it was a patch of mine that introduces LD_IS_{GNU,LLVM} to xen/Kconfig (Acked by someone else) and then you use it here? I realize this is tricking you, but I would rather get this unblocked if possible. While I can agree to test for features in Makefiles closer to the place where they are used, I don't like doing the version checks in Makefiles, much less arch-specific ones. Thanks, Roger.
On 14.03.2022 10:05, Roger Pau Monné wrote: > On Mon, Mar 14, 2022 at 08:38:52AM +0100, Jan Beulich wrote: >> On 11.03.2022 16:07, Roger Pau Monné wrote: >>> On Fri, Mar 11, 2022 at 03:55:57PM +0100, Jan Beulich wrote: >>>> On 11.03.2022 15:34, Roger Pau Monné wrote: >>>>> On Fri, Mar 11, 2022 at 02:28:40PM +0100, Jan Beulich wrote: >>>>>> Support for this construct was added in 2.22 only. Avoid the need to >>>>>> introduce logic to probe for linker script capabilities by (ab)using the >>>>>> probe for a command line option having appeared at about the same time. >>>>>> >>>>>> Fixes: 4b7fd8153ddf ("x86: fold sections in final binaries") >>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>>>> --- >>>>>> v2: Always define HAVE_LD_SORT_BY_INIT_PRIORITY when using LLVM ld. >>>>>> >>>>>> --- a/xen/arch/x86/arch.mk >>>>>> +++ b/xen/arch/x86/arch.mk >>>>>> @@ -73,6 +73,16 @@ ifeq ($(CONFIG_UBSAN),y) >>>>>> $(call cc-option-add,CFLAGS_UBSAN,CC,-fno-sanitize=alignment) >>>>>> endif >>>>>> >>>>>> +ifeq ($(call success,$(LD) --version | head -n 1 | grep -q "GNU ld"),y) >>>>> >>>>> You are not going to like this, but I think this should live in >>>>> xen/Kconfig together with CC_IS_{GCC,CLANG}, ie: LD_IS_GNU or similar. >>>>> >>>>> It's possible we will need this in the future in other places, so >>>>> having it in Kconfig makes sense. >>>> >>>> Despite me not liking this (until, as said elsewhere, we've properly >>>> settled on either approach) I did actually consider doing like you >>>> suggest. But: I would have to introduce there more than I need here, >>>> just for consistency's sake, and I wouldn't have a way to test the >>>> LLD part of it (I did check - none of the distros where I chose to >>>> install Clang offer the linker). I realize I could ask you to help >>>> with the testing, but then the first point would remain. I'd prefer >>>> if for this simple build fix it was okay to go the old fashioned >>>> route ... >>> >>> I would be fine with you just introducing LD_IS_GNU. That's all we >>> need so far. We can introduce LD_IS_LLVM if/when required. I prefer >>> that asymmetry rather than doing the detection here. >> >> Yet I'm not happy to go this route. I'm only willing to do this >> consistently, but that in turn not without us having formally sat down >> and discussed the overall model to use. The only short term alternative >> I see is to go back to SORT() unilaterally, hoping that for now >> different priorities won't be encountered. > > Would you be fine if it was a patch of mine that introduces > LD_IS_{GNU,LLVM} to xen/Kconfig (Acked by someone else) and then you > use it here? > > I realize this is tricking you, but I would rather get this unblocked > if possible. Well, yes, if the construct had been there, I certainly would have used it (somewhat hesitantly maybe). Jan
--- a/xen/arch/x86/arch.mk +++ b/xen/arch/x86/arch.mk @@ -73,6 +73,16 @@ ifeq ($(CONFIG_UBSAN),y) $(call cc-option-add,CFLAGS_UBSAN,CC,-fno-sanitize=alignment) endif +ifeq ($(call success,$(LD) --version | head -n 1 | grep -q "GNU ld"),y) +# While not much better than going by raw GNU ld version, utilize that the +# feature we're after has appeared in the same release as the +# --print-output-format command line option. +AFLAGS-$(call ld-option,--print-output-format) += -DHAVE_LD_SORT_BY_INIT_PRIORITY +else +# Assume all versions of LLD support this. +AFLAGS += -DHAVE_LD_SORT_BY_INIT_PRIORITY +endif + ifneq ($(CONFIG_PV_SHIM_EXCLUSIVE),y) efi-check := arch/x86/efi/check --- a/xen/arch/x86/xen.lds.S +++ b/xen/arch/x86/xen.lds.S @@ -30,6 +30,10 @@ ENTRY(start_pa) # define SECTION_ALIGN PAGE_SIZE #endif +#ifndef HAVE_LD_SORT_BY_INIT_PRIORITY +# define SORT_BY_INIT_PRIORITY SORT +#endif + OUTPUT_FORMAT(FORMAT, FORMAT, FORMAT) OUTPUT_ARCH(i386:x86-64)
Support for this construct was added in 2.22 only. Avoid the need to introduce logic to probe for linker script capabilities by (ab)using the probe for a command line option having appeared at about the same time. Fixes: 4b7fd8153ddf ("x86: fold sections in final binaries") Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v2: Always define HAVE_LD_SORT_BY_INIT_PRIORITY when using LLVM ld.