Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incremental linking of spec segments #1204

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
+442 −715
Draft

Incremental linking of spec segments #1204

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d7c2fb8
segments
glankk Feb 23, 2022
0f5bf82
Moved relocation generation to the segment objects
Thar0 Apr 24, 2022
30497e9
Makefile tweaks, remove -fno-merge-constants gcc flag
Thar0 Apr 24, 2022
58d059b
Make relocations only for segments prefixed with ovl_
Thar0 Apr 24, 2022
50d9496
Small format fix
Thar0 Apr 24, 2022
70d913a
Minimalistic doc in usage
Dragorn421 Apr 25, 2022
dff1ce2
Do not link the .ovl section if the segment isn't an overlay
Dragorn421 Apr 25, 2022
9910cb4
Define SegmentStart outside the segment section
Dragorn421 Apr 25, 2022
bba1ed7
Add empty lines around segment end rom symbols
Dragorn421 Apr 25, 2022
ad30674
Merge pull request #7 from Dragorn421/segments_mkldscript
Thar0 May 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 40 additions & 24 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ endif
ASFLAGS := -march=vr4300 -32 -Iinclude

ifeq ($(COMPILER),gcc)
CFLAGS += -G 0 -nostdinc $(INC) -DAVOID_UB -march=vr4300 -mfix4300 -mabi=32 -mno-abicalls -mdivide-breaks -fno-zero-initialized-in-bss -fno-toplevel-reorder -ffreestanding -fno-common -fno-merge-constants -mno-explicit-relocs -mno-split-addresses $(CHECK_WARNINGS) -funsigned-char
CFLAGS += -G 0 -nostdinc $(INC) -DAVOID_UB -march=vr4300 -mfix4300 -mabi=32 -mno-abicalls -mdivide-breaks -fno-zero-initialized-in-bss -fno-toplevel-reorder -ffreestanding -fno-common -mno-explicit-relocs -mno-split-addresses $(CHECK_WARNINGS) -funsigned-char
MIPS_VERSION := -mips3
else
# we support Microsoft extensions such as anonymous structs, which the compiler does support but warns for their usage. Surpress the warnings with -woff.
Expand Down Expand Up @@ -165,11 +165,17 @@ O_FILES := $(foreach f,$(S_FILES:.s=.o),build/$f) \
$(foreach f,$(C_FILES:.c=.o),build/$f) \
$(foreach f,$(wildcard baserom/*),build/$f.o)

OVL_RELOC_FILES := $(shell $(CPP) $(CPPFLAGS) $(SPEC) | grep -o '[^"]*_reloc.o' )
SEGMENTS := $(shell $(CPP) $(CPPFLAGS) $(SPEC) | grep -o '^[ \t]*name[ \t]\+".\+"' | sed 's/.*"\(.*\)".*/\1/g' )
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this because it assumes things about the not-well-defined spec syntax, which makes it look really hacky.
But as I said before I personally don't even like the current spec syntax in the first place, so I'm biased here.

Still this could be improved by making a spec.c-based tool, that way all assumptions on the spec syntax are at least contained in spec.c, but it may be more trouble than it's worth.
I would be okay with keeping the grep | sed if you think it's fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A spec.c based tool would probably be better, but my lack of knowledge on how to make a pipe-compatible program stopped me from pursuing this (I've read that it's as easy as reading from stdin but cat spec | program did not give expected results when I tried this...)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
SEGMENTS := $(shell $(CPP) $(CPPFLAGS) $(SPEC) | grep -o '^[ \t]*name[ \t]\+".\+"' | sed 's/.*"\(.*\)".*/\1/g' )
SEGMENTS := $(shell $(CPP) $(CPPFLAGS) $(SPEC) | grep -o '^[ \t]*name[ \t]\+".\+"' | sed 's/.*"\(.*\)".*/\1/g')

SEGMENTS_OVL := $(filter ovl_%,$(SEGMENTS))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like that overlays would be defined by segment name, but we already have/had this kind of name-dependant behavior in master (OVL_RELOC_FILES and _reloc in general) so whatever.

A possibility would be to add a spec directive for them but again may be more trouble than worth.

Copy link
Contributor Author

@Thar0 Thar0 Apr 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really like resorting to the name either, but left it at this since as you pointed out there was already name dependence. In an earlier commit I was toying with the idea of using flags OVL as a means to distinguish overlays, but I dropped this for two reasons. First, to implement this cleanly it would need a tool that can receive the spec through a pipe (see prior comment on why this is apparently a difficulty in it's own right), and second it would be a change to the spec format itself as this flag doesn't exist.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is changing the spec format itself an issue? (besides being more work than may be worth)
We already expanded it in the past afaik (at the very least include_data_with_rodata was surely not part of the "original", whatever the original is, and probably other directives)

SEGMENT_DIR := build/segments
SEGMENT_OBJECTS := $(SEGMENTS:%=$(SEGMENT_DIR)/%.o)
SEGMENT_RELOCS := $(SEGMENTS_OVL:%=$(SEGMENT_DIR)/%.reloc.o)
SEGMENT_SCRIPTS := $(SEGMENT_OBJECTS:.o=.ld)
SEGMENT_DEPENDS := $(SEGMENT_OBJECTS:.o=.d)

# Automatic dependency files
# (Only asm_processor dependencies and reloc dependencies are handled for now)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# (Only asm_processor dependencies and reloc dependencies are handled for now)
# (Only asm_processor dependencies are handled for now)

it's unclear what this comment is for but I assume it's about DEP_FILES
maybe mention relocs somewhere else
but idk an actual write up on the build system would be a better idea...

DEP_FILES := $(O_FILES:.o=.asmproc.d) $(OVL_RELOC_FILES:.o=.d)
DEP_FILES := $(O_FILES:.o=.asmproc.d)


TEXTURE_FILES_PNG := $(foreach dir,$(ASSET_BIN_DIRS),$(wildcard $(dir)/*.png))
Expand All @@ -178,7 +184,7 @@ TEXTURE_FILES_OUT := $(foreach f,$(TEXTURE_FILES_PNG:.png=.inc.c),build/$f) \
$(foreach f,$(TEXTURE_FILES_JPG:.jpg=.jpg.inc.c),build/$f) \

# create build directories
$(shell mkdir -p build/baserom build/assets/text $(foreach dir,$(SRC_DIRS) $(ASM_DIRS) $(ASSET_BIN_DIRS),build/$(dir)))
$(shell mkdir -p build/baserom build/assets/text $(SEGMENT_DIR) $(foreach dir,$(SRC_DIRS) $(ASM_DIRS) $(ASSET_BIN_DIRS),build/$(dir)))

ifeq ($(COMPILER),ido)
build/src/code/fault.o: CFLAGS += -trapuv
Expand Down Expand Up @@ -262,30 +268,44 @@ test: $(ROM)
$(ROM): $(ELF)
$(ELF2ROM) -cic 6105 $< $@

$(ELF): $(TEXTURE_FILES_OUT) $(ASSET_FILES_OUT) $(O_FILES) $(OVL_RELOC_FILES) build/ldscript.txt build/undefined_syms.txt
$(LD) -T build/undefined_syms.txt -T build/ldscript.txt --no-check-sections --accept-unknown-input-arch --emit-relocs -Map build/z64.map -o $@
$(ELF): $(SEGMENT_OBJECTS) $(SEGMENT_RELOCS) build/ldscript.txt build/undefined_syms.txt
$(LD) -T build/undefined_syms.txt -T build/ldscript.txt --emit-relocs -Map build/z64.map -o $@

## Order-only prerequisites
# These ensure e.g. the O_FILES are built before the OVL_RELOC_FILES.
# The intermediate phony targets avoid quadratically-many dependencies between the targets and prerequisites.
build/undefined_syms.txt: undefined_syms.txt
$(CPP) $(CPPFLAGS) $< > $@

o_files: $(O_FILES)
$(OVL_RELOC_FILES): | o_files
build/$(SPEC): $(SPEC)
$(CPP) $(CPPFLAGS) $< > $@

asset_files: $(TEXTURE_FILES_OUT) $(ASSET_FILES_OUT)
$(O_FILES): | asset_files
build/ldscript.txt: build/$(SPEC)
$(MKLDSCRIPT) -r $< $(SEGMENT_DIR) $@

.PHONY: o_files asset_files
$(SEGMENT_SCRIPTS): build/$(SPEC)
$(SEGMENT_SCRIPTS): %.ld: %.d
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does each segment's linker script depend on the segment's dependency file? Isn't the dependency on the (preprocessed) spec enough?

Copy link
Contributor

@glankk glankk Apr 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's because mkldscript generates both the dependency file and the script in the same invocation. Having a dependency relation between the two is the neatest way to communicate this to make.

Copy link
Collaborator

@Dragorn421 Dragorn421 Apr 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand, they are indeed both generated at the same time, so why would one depend on the other?
Also, typically (always) .ld/.d will be newer than build/$(SPEC) and the .lds already depend on build/$(SPEC), so the .d would also be up to date?

Also unrelated to my question but even if this is needed, it assumes the .ld is written before after the .d, or "not too long" after before (whatever make's or the system's definition of "not too long" is)
(currently mkldscript writes the .ld after before the .d time-wise)

Copy link
Contributor

@glankk glankk Apr 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you remove this dependency relation, then make would no longer know how to generate .ld. You would have to provide a recipe for it, which would be the same as .d. This would cause two invocations of mkldscript to occur, meaning both files would be generated twice, a waste of time. Having .d be newer than .ld is not a problem, because .ld does not have a recipe and thus will not be regenerated, even if it is considered "out of date" in a sense.

Edit: In practice this doesn't really make a difference, it would really only be necessary if you removed the -include $(SEGMENT_DEPENDS) line and then tried to do something like make build/segments/code.ld.


$(SEGMENT_DEPENDS): build/$(SPEC)
$(MKLDSCRIPT) -s $< $(SEGMENT_DIR) $(@:$(SEGMENT_DIR)/%.d=%)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imo the makefile should pass the .d and .ld locations to mkldscript as well, instead of mkldscript deriving them from the segment name. That way paths are all contained in the makefile


build/$(SPEC): $(SPEC)
$(CPP) $(CPPFLAGS) $< > $@
ifeq ($(MAKECMDGOALS),$(filter-out clean assetclean distclean setup,$(MAKECMDGOALS)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An explanatory comment could be useful here. This conditional is to prevent the generation of segment dependency files for targets that don't touch any of the segment objects. In the case of cleaning targets, omitting this would cause these files to be generated just to be immediately removed again. Unfortunately I don't know of a better way to solve this.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also causes weird issues when using exotic make calls like make assetclean all, but it's probably not worth worrying about.

Having a different makefile for the "clean" targets, or just a script given that it's just doing rms, sounds like a good fix 😄 but, as long as it works as is...

-include $(SEGMENT_DEPENDS)
endif

build/ldscript.txt: build/$(SPEC)
$(MKLDSCRIPT) $< $@
$(SEGMENT_DIR)/%.reloc.o: $(SEGMENT_DIR)/%.o
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would probably split this into two recipes unless it negatively affects the build time.

$(FADO) $< -n $(<F:.o=) -o $(@:.o=.s)
$(AS) $(ASFLAGS) $(@:.o=.s) -o $@

$(SEGMENT_OBJECTS): %.o: %.ld %.d
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here too I don't understand why there needs to be a dependency on the .d

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's pretty subtle, but .d is necessary in order for make to know whether .o is up to date or not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again probably doesn't make a difference in practice, but I do think it's more correct. I find it a bit confusing myself though tbh even though I wrote it and it made sense to me at the time.

$(LD) -r -T $< --accept-unknown-input-arch -Map $(@:.o=.map) -o $@
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally really think we should make the makefile as verbose as can be, both for readability (more like "understandability") and easier debugging.

Suggested change
$(LD) -r -T $< --accept-unknown-input-arch -Map $(@:.o=.map) -o $@
$(LD) --relocatable -T $< --accept-unknown-input-arch -Map $(@:.o=.map) -o $@

Also, you're keeping --accept-unknown-input-arch here but removing from the "main" ld call? What is its purpose here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All objects that go into the main ld call have been partially linked at that time, which means they all have an architecture as defined in the segment linker script. Some of the objects that go into the segments have a "generic" architecture which causes ld to complain without this option.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And "generic architecture" objects are typically (and, in this repo, I guess exclusively) the ones that are objcopy'ed from binary

$ mips-linux-gnu-objdump -f build/src/overlays/actors/ovl_En_Daiku/z_en_daiku.o

build/src/overlays/actors/ovl_En_Daiku/z_en_daiku.o:     file format elf32-tradbigmips
architecture: mips:6000, flags 0x00000011:
HAS_RELOC, HAS_SYMS
start address 0x00000000

$ mips-linux-gnu-objdump -f build/baserom/bump_texture_static.o

build/baserom/bump_texture_static.o:     file format elf32-big
architecture: UNKNOWN!, flags 0x00000010:
HAS_SYMS
start address 0x00000000

Thanks :)


## Order-only prerequisites
# These ensure e.g. the asset include files are built before the files that include them
# The intermediate phony targets avoid quadratically-many dependencies between the targets and prerequisites.

asset_files: $(TEXTURE_FILES_OUT) $(ASSET_FILES_OUT)
$(O_FILES): | asset_files

.PHONY: asset_files

build/undefined_syms.txt: undefined_syms.txt
$(CPP) $(CPPFLAGS) $< > $@

build/baserom/%.o: baserom/%
$(OBJCOPY) -I binary -O elf32-big $< $@
Expand Down Expand Up @@ -331,10 +351,6 @@ build/src/libultra/libc/llcvt.o: src/libultra/libc/llcvt.c
python3 tools/set_o32abi_bit.py $@
@$(OBJDUMP) -d $@ > $(@:.o=.s)

build/src/overlays/%_reloc.o: build/$(SPEC)
$(FADO) $$(tools/reloc_prereq $< $(notdir $*)) -n $(notdir $*) -o $(@:.o=.s) -M $(@:.o=.d)
$(AS) $(ASFLAGS) $(@:.o=.s) -o $@

build/%.inc.c: %.png
$(ZAPD) btex -eh -tt $(subst .,,$(suffix $*)) -i $< -o $@

Expand Down
Loading