| Message ID | 20240604115932.86596-2-mhartmay@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: small Makefile improvements | expand |
On Tue Jun 4, 2024 at 9:59 PM AEST, Marc Hartmayer wrote: > It's unusual to create multiple files in one target rule, and it's even more > unusual to create an ELF file with a `.gbin` file extension first, and then > overwrite it in the next step. It might even lead to errors as the input file > path is also used as the output file path - but this depends on the objcopy > implementation. Therefore, create an extra target for the ELF files and list it > as a prerequisite for the *.gbin targets. I had some pain trying to figure out another ("pretty printing") patch that changed some s390x/Makefile because of this. As far as I can tell it looks good. Reviewed-by: Nicholas Piggin <npiggin@gmail.com> > > Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> > --- > s390x/Makefile | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/s390x/Makefile b/s390x/Makefile > index 23342bd64f44..784818b2883e 100644 > --- a/s390x/Makefile > +++ b/s390x/Makefile > @@ -153,14 +153,18 @@ $(SNIPPET_DIR)/asm/%.o: $(SNIPPET_DIR)/asm/%.S $(asm-offsets) > $(SNIPPET_DIR)/c/%.o: $(SNIPPET_DIR)/c/%.c $(asm-offsets) > $(CC) $(CFLAGS) -c -nostdlib -o $@ $< > > -$(SNIPPET_DIR)/asm/%.gbin: $(SNIPPET_DIR)/asm/%.o $(SNIPPET_DIR)/asm/flat.lds > +$(SNIPPET_DIR)/asm/%.elf: $(SNIPPET_DIR)/asm/%.o $(SNIPPET_DIR)/asm/flat.lds > $(CC) $(LDFLAGS) -o $@ -T $(SNIPPET_DIR)/asm/flat.lds $< > - $(OBJCOPY) -O binary -j ".rodata" -j ".lowcore" -j ".text" -j ".data" -j ".bss" --set-section-flags .bss=alloc,load,contents $@ $@ > + > +$(SNIPPET_DIR)/asm/%.gbin: $(SNIPPET_DIR)/asm/%.elf > + $(OBJCOPY) -O binary -j ".rodata" -j ".lowcore" -j ".text" -j ".data" -j ".bss" --set-section-flags .bss=alloc,load,contents $< $@ > truncate -s '%4096' $@ > > -$(SNIPPET_DIR)/c/%.gbin: $(SNIPPET_DIR)/c/%.o $(snippet_lib) $(FLATLIBS) $(SNIPPET_DIR)/c/flat.lds > +$(SNIPPET_DIR)/c/%.elf: $(SNIPPET_DIR)/c/%.o $(snippet_lib) $(FLATLIBS) $(SNIPPET_DIR)/c/flat.lds > $(CC) $(LDFLAGS) -o $@ -T $(SNIPPET_DIR)/c/flat.lds $< $(snippet_lib) $(FLATLIBS) > - $(OBJCOPY) -O binary -j ".rodata" -j ".lowcore" -j ".text" -j ".data" -j ".bss" --set-section-flags .bss=alloc,load,contents $@ $@ > + > +$(SNIPPET_DIR)/c/%.gbin: $(SNIPPET_DIR)/c/%.elf > + $(OBJCOPY) -O binary -j ".rodata" -j ".lowcore" -j ".text" -j ".data" -j ".bss" --set-section-flags .bss=alloc,load,contents $< $@ > truncate -s '%4096' $@ > > %.hdr: %.gbin $(HOST_KEY_DOCUMENT)
On Wed, Jun 05, 2024 at 11:21 AM +1000, "Nicholas Piggin" <npiggin@gmail.com> wrote: > On Tue Jun 4, 2024 at 9:59 PM AEST, Marc Hartmayer wrote: >> It's unusual to create multiple files in one target rule, and it's even more >> unusual to create an ELF file with a `.gbin` file extension first, and then >> overwrite it in the next step. It might even lead to errors as the input file >> path is also used as the output file path - but this depends on the objcopy >> implementation. Therefore, create an extra target for the ELF files and list it >> as a prerequisite for the *.gbin targets. > > I had some pain trying to figure out another ("pretty printing") patch > that changed some s390x/Makefile because of this. As far as I can tell > it looks good. Hehe yes. Thomas sent me the following error message: /usr/bin/s390x-linux-gnu-ld: warning: s390x/snippets/c/mvpg-snippet.gbin has a LOAD segment with RWX permissions …and at first this was totally confusing until I’ve looked at the code… :) > > Reviewed-by: Nicholas Piggin <npiggin@gmail.com> Thanks. […snip]
On 6/4/24 1:59 PM, Marc Hartmayer wrote: > It's unusual to create multiple files in one target rule, and it's even more > unusual to create an ELF file with a `.gbin` file extension first, and then > overwrite it in the next step. It might even lead to errors as the input file > path is also used as the output file path - but this depends on the objcopy > implementation. Therefore, create an extra target for the ELF files and list it > as a prerequisite for the *.gbin targets. > > Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
On 6/4/24 1:59 PM, Marc Hartmayer wrote: > It's unusual to create multiple files in one target rule, and it's even more > unusual to create an ELF file with a `.gbin` file extension first, and then > overwrite it in the next step. It might even lead to errors as the input file > path is also used as the output file path - but this depends on the objcopy > implementation. Therefore, create an extra target for the ELF files and list it > as a prerequisite for the *.gbin targets. > > Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> I've picked this one but it's unlikely that I'll pick the other patches in the series. Thanks for improving the makefile and fixing my mistakes :)
On Mon, Jul 29, 2024 at 09:00 AM +0200, Janosch Frank <frankja@linux.ibm.com> wrote: > On 6/4/24 1:59 PM, Marc Hartmayer wrote: >> It's unusual to create multiple files in one target rule, and it's even more >> unusual to create an ELF file with a `.gbin` file extension first, and then >> overwrite it in the next step. It might even lead to errors as the input file >> path is also used as the output file path - but this depends on the objcopy >> implementation. Therefore, create an extra target for the ELF files and list it >> as a prerequisite for the *.gbin targets. >> >> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> > > I've picked this one but it's unlikely that I'll pick the other patches > in the series. Thanks for improving the makefile and fixing my > mistakes :) Thanks for the r-b and fine with me. The other two patches are not as useful and would make s390x even more different from other architectures with little to no real benefit. >
diff --git a/s390x/Makefile b/s390x/Makefile index 23342bd64f44..784818b2883e 100644 --- a/s390x/Makefile +++ b/s390x/Makefile @@ -153,14 +153,18 @@ $(SNIPPET_DIR)/asm/%.o: $(SNIPPET_DIR)/asm/%.S $(asm-offsets) $(SNIPPET_DIR)/c/%.o: $(SNIPPET_DIR)/c/%.c $(asm-offsets) $(CC) $(CFLAGS) -c -nostdlib -o $@ $< -$(SNIPPET_DIR)/asm/%.gbin: $(SNIPPET_DIR)/asm/%.o $(SNIPPET_DIR)/asm/flat.lds +$(SNIPPET_DIR)/asm/%.elf: $(SNIPPET_DIR)/asm/%.o $(SNIPPET_DIR)/asm/flat.lds $(CC) $(LDFLAGS) -o $@ -T $(SNIPPET_DIR)/asm/flat.lds $< - $(OBJCOPY) -O binary -j ".rodata" -j ".lowcore" -j ".text" -j ".data" -j ".bss" --set-section-flags .bss=alloc,load,contents $@ $@ + +$(SNIPPET_DIR)/asm/%.gbin: $(SNIPPET_DIR)/asm/%.elf + $(OBJCOPY) -O binary -j ".rodata" -j ".lowcore" -j ".text" -j ".data" -j ".bss" --set-section-flags .bss=alloc,load,contents $< $@ truncate -s '%4096' $@ -$(SNIPPET_DIR)/c/%.gbin: $(SNIPPET_DIR)/c/%.o $(snippet_lib) $(FLATLIBS) $(SNIPPET_DIR)/c/flat.lds +$(SNIPPET_DIR)/c/%.elf: $(SNIPPET_DIR)/c/%.o $(snippet_lib) $(FLATLIBS) $(SNIPPET_DIR)/c/flat.lds $(CC) $(LDFLAGS) -o $@ -T $(SNIPPET_DIR)/c/flat.lds $< $(snippet_lib) $(FLATLIBS) - $(OBJCOPY) -O binary -j ".rodata" -j ".lowcore" -j ".text" -j ".data" -j ".bss" --set-section-flags .bss=alloc,load,contents $@ $@ + +$(SNIPPET_DIR)/c/%.gbin: $(SNIPPET_DIR)/c/%.elf + $(OBJCOPY) -O binary -j ".rodata" -j ".lowcore" -j ".text" -j ".data" -j ".bss" --set-section-flags .bss=alloc,load,contents $< $@ truncate -s '%4096' $@ %.hdr: %.gbin $(HOST_KEY_DOCUMENT)
It's unusual to create multiple files in one target rule, and it's even more unusual to create an ELF file with a `.gbin` file extension first, and then overwrite it in the next step. It might even lead to errors as the input file path is also used as the output file path - but this depends on the objcopy implementation. Therefore, create an extra target for the ELF files and list it as a prerequisite for the *.gbin targets. Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- s390x/Makefile | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)