Re: [XEN PATCH 07/15] build: move XEN_HAS_BUILD_ID out of Config.mk
On Thu, May 25, 2023 at 01:56:53PM +0200, Jan Beulich wrote: > On 23.05.2023 18:38, Anthony PERARD wrote: > > Whether or not the linker can do build id is only used by the > > hypervisor build, so move that there. > > > > Rename $(build_id_linker) to $(XEN_LDFLAGS_BUILD_ID) as this is a > > better name to be exported as to use the "XEN_*" namespace. > > > > Also update XEN_TREEWIDE_CFLAGS so flags can be used for > > arch/x86/boot/ CFLAGS_x86_32 > > > > Beside a reordering of the command line where CFLAGS is used, there > > shouldn't be any other changes. > > > > Signed-off-by: Anthony PERARD > > Reviewed-by: Jan Beulich > with one nit: > > > --- a/xen/scripts/Kbuild.include > > +++ b/xen/scripts/Kbuild.include > > @@ -91,6 +91,9 @@ cc-ifversion = $(shell [ $(CONFIG_GCC_VERSION)0 $(1) > > $(2)000 ] && echo $(3) || e > > > > clang-ifversion = $(shell [ $(CONFIG_CLANG_VERSION)0 $(1) $(2)000 ] && > > echo $(3) || echo $(4)) > > > > +ld-ver-build-id = $(shell $(1) --build-id 2>&1 | \ > > + grep -q build-id && echo n || echo y) > > I realize you only move this line, but I think we want indentation here > to improve at this occasion: > > ld-ver-build-id = $(shell $(1) --build-id 2>&1 | \ > grep -q build-id && echo n || echo y) > > I'll be happy to adjust while committing. Which raises the question: Is > there any dependency here on earlier patches in the series? It doesn't > look so to me, but I may easily be overlooking something. (Of course > first further arch maintainer acks would be needed.) I don't see any dependency on earlier patch, so it looks fine to apply earlier. Thanks. > > --- a/xen/test/livepatch/Makefile > > +++ b/xen/test/livepatch/Makefile > > @@ -37,7 +37,7 @@ $(obj)/modinfo.o: > > > > # > > # This target is only accessible if CONFIG_LIVEPATCH is defined, which > > -# depends on $(build_id_linker) being available. Hence we do not > > +# depends on $(XEN_LDFLAGS_BUILD_ID) being available. Hence we do not > > # need any checks. > > As an aside, I'm a little confused by "is only accessible" here. I don't > see how CONFIG_LIVEPATCH controls reachability. At the very least the > parent dir's Makefile doesn't use CONFIG_LIVEPATCH at all. Yes. `make tests` works just fine without CONFIG_LIVEPATCH. Even when that comment was introduce, it didn't seems like there were anything preventing the target from been run. The only thing is that `make tests` doesn't build the tests if xen-syms was built without build_id, so the status of CONFIG_LIVEPATCH doesn't seems to matter. Thanks, -- Anthony PERARD
Re: [XEN PATCH 07/15] build: move XEN_HAS_BUILD_ID out of Config.mk
On 23.05.2023 18:38, Anthony PERARD wrote: > Whether or not the linker can do build id is only used by the > hypervisor build, so move that there. > > Rename $(build_id_linker) to $(XEN_LDFLAGS_BUILD_ID) as this is a > better name to be exported as to use the "XEN_*" namespace. > > Also update XEN_TREEWIDE_CFLAGS so flags can be used for > arch/x86/boot/ CFLAGS_x86_32 > > Beside a reordering of the command line where CFLAGS is used, there > shouldn't be any other changes. > > Signed-off-by: Anthony PERARD Reviewed-by: Jan Beulich with one nit: > --- a/xen/scripts/Kbuild.include > +++ b/xen/scripts/Kbuild.include > @@ -91,6 +91,9 @@ cc-ifversion = $(shell [ $(CONFIG_GCC_VERSION)0 $(1) > $(2)000 ] && echo $(3) || e > > clang-ifversion = $(shell [ $(CONFIG_CLANG_VERSION)0 $(1) $(2)000 ] && echo > $(3) || echo $(4)) > > +ld-ver-build-id = $(shell $(1) --build-id 2>&1 | \ > + grep -q build-id && echo n || echo y) I realize you only move this line, but I think we want indentation here to improve at this occasion: ld-ver-build-id = $(shell $(1) --build-id 2>&1 | \ grep -q build-id && echo n || echo y) I'll be happy to adjust while committing. Which raises the question: Is there any dependency here on earlier patches in the series? It doesn't look so to me, but I may easily be overlooking something. (Of course first further arch maintainer acks would be needed.) > --- a/xen/test/livepatch/Makefile > +++ b/xen/test/livepatch/Makefile > @@ -37,7 +37,7 @@ $(obj)/modinfo.o: > > # > # This target is only accessible if CONFIG_LIVEPATCH is defined, which > -# depends on $(build_id_linker) being available. Hence we do not > +# depends on $(XEN_LDFLAGS_BUILD_ID) being available. Hence we do not > # need any checks. As an aside, I'm a little confused by "is only accessible" here. I don't see how CONFIG_LIVEPATCH controls reachability. At the very least the parent dir's Makefile doesn't use CONFIG_LIVEPATCH at all. Jan
Re: [XEN PATCH 07/15] build: move XEN_HAS_BUILD_ID out of Config.mk
> On 23 May 2023, at 17:38, Anthony PERARD wrote: > > Whether or not the linker can do build id is only used by the > hypervisor build, so move that there. > > Rename $(build_id_linker) to $(XEN_LDFLAGS_BUILD_ID) as this is a > better name to be exported as to use the "XEN_*" namespace. > > Also update XEN_TREEWIDE_CFLAGS so flags can be used for > arch/x86/boot/ CFLAGS_x86_32 > > Beside a reordering of the command line where CFLAGS is used, there > shouldn't be any other changes. > > Signed-off-by: Anthony PERARD Seems good to me! Reviewed-by: Luca Fancellu Tested-by: Luca Fancellu
