Re: [PATCH] Arm: do a 4th linking pass if necessary

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Thu, May 21, 2026 at 08:08:44AM +0200, Jan Beulich wrote:
> On 20.05.2026 18:03, Anthony PERARD wrote:
> > On Wed, May 20, 2026 at 01:53:34PM +0200, Jan Beulich wrote:
> >> +  then \
> >> +          $(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds $< $(build_id_linker) \
> >> +              $(dot-target).2.o -o $(dot-target).2; \
> >> +          $(NM) -pa --format=sysv $(dot-target).2 \
> >> +                  | $(objtree)/tools/symbols $(all_symbols) --sysv --sort 
> >> \
> >> +                  > $(dot-target).3.S; \
> >> +          $(MAKE) $(build)=$(@D) $(dot-target).3.o; \
> > 
> > This new block ignore all errors, from LD, NM and MAKE. We want
> > a `set -e` before the if.
> 
> Hmm, perhaps I should add that, yes, albeit ...
> 
> >> +          $(call compare-symbol-tables, $(dot-target).2.o, 
> >> $(dot-target).3.o); \
> > 
> > At least, an error returned by `diff` in that macro should be taken into
> > account, for now.
> 
> ... I expect this would fail if there was an earlier error.

Yes, but that's fragile, and that's not how `make` behave. It's better if
every command behave the same way, that is the recipe fails on the first
command that fail. So adding `set -e` would be useful.

> >> --- a/xen/scripts/Kbuild.include
> >> +++ b/xen/scripts/Kbuild.include
> >> @@ -65,7 +65,7 @@ define compare-symbol-tables
> >>      $(OBJDUMP) -t $(@D)/.cst.$$$$ > $(1).sym; \
> >>      ln -f $(2) $(@D)/.cst.$$$$; \
> >>      $(OBJDUMP) -t $(@D)/.cst.$$$$ > $(2).sym; \
> >> -    rm -f $(@D)/.cst.$$$$
> >> +    rm -f $(@D)/.cst.$$$$; \
> >>      diff -u $(1).sym $(2).sym
> > 
> > This macro is missing `set -e`, if both OBJDUMP command fails and create
> > an empty file, `diff` will return success.
> 
> Whether to have "set -e" here is an independent question, I guess. To avoid
> the case you mention, maybe better
> 
>       $(OBJDUMP) -t $(@D)/.cst.$$$$ > $(1).sym || rm -f $(1).sym; \
> 
> ?

That sounds fine. (Replacing all the `;` by `&&` would work too.)

> > But looks like `set -e` in
> > this macro isn't going to work in the condition of the `if`.
> 
> Whereas the above would be compatible with both uses, I think.

Yes.


With at lest a `set -e` for the `if` block:
Reviewed-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>

Thanks,


--
Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech