Conversation
subrepo: subdir: "tools/com-plugin" merged: "c4f3ba845" upstream: origin: "git@github.com:Thar0/com-plugin.git" branch: "main" commit: "c4f3ba845" git-subrepo: version: "0.4.6" origin: "https://github.com/ingydotnet/git-subrepo" commit: "110b9eb"
Co-authored-by: Anghelo Carvajal <angheloalf95@gmail.com>
| # Generate and include segment makefile rules for combining .o files into single .plf files for an entire spec segment. | ||
| # Overlay relocations will be generated from these if the spec segment has the OVERLAY flag. | ||
| # If this makefile doesn't exist or if the spec has been modified since make was last ran it will use the rule | ||
| # later on in the file to regenerate this file before including it. The test against MAKECMDGOALS ensures this | ||
| # doesn't happen if we're not running a task that needs these partially linked files; this is especially important | ||
| # for setup since the rule to generate the segment makefile rules requires setup to have ran first. | ||
| SEG_LDFLAGS = -r $(COM_PLUGIN_FLAGS) -T $(@:.plf=.ld) -Map $(@:.plf=.map) | ||
| SEG_VERBOSE = @ | ||
| ifeq ($(MAKECMDGOALS),$(filter-out clean assetclean distclean setup,$(MAKECMDGOALS))) | ||
| include $(SEGMENTS_DIR)/Makefile | ||
| else | ||
| SEGMENT_FILES := | ||
| OVL_SEGMENT_FILES := | ||
| endif |
There was a problem hiding this comment.
This is a bit dense.
So we are now autogenerating a Makefile for those plfs?
There was a problem hiding this comment.
Yeah, the way it works is:
- The makefile reaches this include statement (when a rule is run other than those that are mentioned in the
filter-out) - The makefile checks to see if
$(SEGMENTS_DIR)/Makefileexists. - Since the makefile contains a rule to build
$(SEGMENTS_DIR)/Makefile, if it doesn't exist it will execute that rule and any rules it relies on to make$(SEGMENTS_DIR)/Makefileavailable. - It then includes it and carries on as if it were always there
Generating a makefile from the spec for each plf is the most convenient way to handle dependencies that I can think of that doesn't involve separating by directory (since we can't do that for multiversion anyway when files move between boot and code)
| $(OBJCOPY) --pad-to 0x$$($(OBJDUMP) -t $< | grep _RomSize | cut -d ' ' -f 1) -O binary $< $@ | ||
| $(PYTHON) -m ipl3checksum sum --cic $(CIC) --update $@ |
There was a problem hiding this comment.
woooo, happy to see we are finally ditching elf2rom.
btw, that objdump invocation seems a bit convoluted, it may be nice to have a comment about why and what it is doing.
| # Generates relocations for each overlay after partial linking so that the final | ||
| # link step cannot later insert padding between individual overlay files after | ||
| # relocations have already been calculated. | ||
| $(SEGMENTS_DIR)/%.reloc.o: $(SEGMENTS_DIR)/%.plf | ||
| $(FADO) $< -n $(notdir $*) -o $(@:.o=.s) | ||
| $(POSTPROCESS_OBJ) $(@:.o=.s) | ||
| $(AS) $(ASFLAGS) $(@:.o=.s) -o $@ |
There was a problem hiding this comment.
So now we just pass a single plf file to fado each time we generate the relocs file?
Nice that simplify stuff a lot.
Makefile
Outdated
| # .text unaccounted linker padding | ||
| $(BUILD_DIR)/__pad_text%.o: | ||
| echo ".text; .fill 0x10" | $(AS) $(ASFLAGS) -o $@ |
There was a problem hiding this comment.
What's the advantage of doing this instead of letting the linker handle it instead?
There was a problem hiding this comment.
This is an artefact of my original approach where I used a single generic linker script for all segments. In that instance we can't insert padding in the linker itself, so I switched to files. But since I wasn't able to keep the single linker script approach anyway, I think I'll revert this to handling it in the linker script as before.
| beginseg | ||
| name "makerom" | ||
| // We set the address of the makerom segment as 0x80000400 - 0x1000, since the ROM header and IPL3 together | ||
| // are 0x1000 bytes long and we want the entry code to end up at address 0x80000400. | ||
| address 0x7FFFF400 | ||
| include "$(BUILD_DIR)/src/makerom/rom_header.o" | ||
| include "$(BUILD_DIR)/src/makerom/ipl3.o" | ||
| include "$(BUILD_DIR)/src/makerom/entry.o" | ||
| endseg |
There was a problem hiding this comment.
makerom is done automagically now? How does it work?
I noted the Makefile has some special cases for the makerom files too, are those really needed too?
There was a problem hiding this comment.
I've written a comment in mkldscript.c where I laid out the makerom segment. I had to remove makerom from the spec due to the fake address assignment, it caused objcopy to be unable to write the ROM properly. I don't think we lose anything by doing this, the layout of the makerom segment shouldn't really change. FWIW it's also more correct to how spec files actually looked.
| #if !DEBUG_FEATURES | ||
| include "$(BUILD_DIR)/src/libc64/sprintf.o" | ||
| #endif |
There was a problem hiding this comment.
I'm surprised this extra checks doesn't affect matching.
There was a problem hiding this comment.
It turns out some files were being passed to the linker twice and it was taking the first mention and ignoring the second. This broke partial linking though since two segments can end up with a copy of the files that would then cause final link to complain about multiple definitions.
There was a problem hiding this comment.
It isn't possible to just have all those symbols in the same file?
Does the linker when trying to link symbols not present on the input objects?
There was a problem hiding this comment.
The BSS order is now enforced at partial link rather than final link. The plugin errors if it can't find a mentioned symbol in any of the input files, since this can often point to a user error.
fig02
left a comment
There was a problem hiding this comment.
Cant say I understand every change made here but I agree with things in principle.
I timed full builds and it was negligibly slower with the proposed changes, but I understand the benefit here is faster incremental builds. Just thought I'd add anyway
|
We're so back |
|
@AngheloAlf bump to re-review |
|
Don't fully understand the changes myself either, but one thing I'd prefer to see some time in the future is the |
|
These changes don't have any bearing on whether there should or shouldn't be an overlays directory |
Dragorn421
left a comment
There was a problem hiding this comment.
I am lukewarm though I appreciate the various loose ends this solves and the clean-up
The return of partial linking.
This PR updates the build process to partial link (
ld -r) each spec segment before final link, which stands to improve incremental build times and eliminates various alignment assumptions we've been forced to make in the past. A key part of this setup is that overlay relocations are generated after partial link for spec segments that have been marked withflags OVERLAY, manually including the relocation object file in the spec is no longer needed.This also removes elf2rom and instead uses objcopy to create the final image from the final elf file.
Special thanks to @glankk for help on the first attempt at partial linking, which was valuable for my understanding.
Special thanks to @AngheloAlf for implementing the necessary mapfile-parser features for supporting multiple map files transparently to keep tools working as before.