Incremental linking of spec segments #1204
Conversation
| "\t.debug_pubtypes 0 : { *(.debug_pubtypes) }\n" | ||
| "\t.debug_ranges 0 : { *(.debug_ranges) }\n" | ||
| "\n" | ||
| /* DWARF extensions */ |
There was a problem hiding this comment.
These are DWARF 5 sections.
|
|
||
| build/$(SPEC): $(SPEC) | ||
| $(CPP) $(CPPFLAGS) $< > $@ | ||
| ifeq ($(MAKECMDGOALS),$(filter-out clean assetclean distclean setup,$(MAKECMDGOALS))) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
|
|
||
| build/ldscript.txt: build/$(SPEC) | ||
| $(MKLDSCRIPT) $< $@ | ||
| $(SEGMENT_DIR)/%.reloc.o: $(SEGMENT_DIR)/%.o |
There was a problem hiding this comment.
I would probably split this into two recipes unless it negatively affects the build time.
| @@ -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' ) | |||
There was a problem hiding this comment.
| 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') |
There was a problem hiding this comment.
glankk this is rookie nitpicking, get on my level 😏
build/ldscript.txt is incredibly more readable now, great work. I'll try to do timings later for the performance side of this.
And I didn't realize reading the makefile but looking in build/segments/ we now have separate linker maps for every segment 🤩
I think tools/reloc_prereq can be removed (or repurposed)
For anyone interested in reviewing, you may need the make manual 😄
https://www.gnu.org/software/make/manual/make.html
(unless you're a pro already 😎, I'm not)
| @@ -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' ) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...)
| @@ -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' ) | |||
| SEGMENTS_OVL := $(filter ovl_%,$(SEGMENTS)) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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_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) |
There was a problem hiding this comment.
| # (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...
|
|
||
| .PHONY: o_files asset_files | ||
| $(SEGMENT_SCRIPTS): build/$(SPEC) | ||
| $(SEGMENT_SCRIPTS): %.ld: %.d |
There was a problem hiding this comment.
Why does each segment's linker script depend on the segment's dependency file? Isn't the dependency on the (preprocessed) spec enough?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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=%) |
There was a problem hiding this comment.
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
tools/mkldscript.c
Outdated
| /* overlay relocation section */ | ||
| fprintf(f, "\t\t_%sSegmentOvlStart = .;\n", seg->name); | ||
|
|
||
| if (strncmp(seg->name, "ovl_", 4) == 0) | ||
| fprintf(f, "\t\t%s/%s.reloc.o (.ovl)\n", dir, seg->name); |
There was a problem hiding this comment.
I think we can do without the SegmentOvlStart & co symbols for segments that are not overlays, those symbols are useless anyway (still may be useful for actual overlays for debugging though)
There was a problem hiding this comment.
I was thinking to do this but I wasn't sure if it's worth it since the OvlEnd symbol is used in calculation of the segment rom size
There was a problem hiding this comment.
So the obvious solution is to switch between
_%sSegmentRomSize = ABSOLUTE(_%sSegmentOvlEnd - _%sSegmentTextStart);
and
_%sSegmentRomSize = ABSOLUTE(_%sSegmentRoDataEnd - _%sSegmentTextStart);
as appropriate
Which looked bad to me at first, but tbh may not be so bad of an idea. mkldscript is fully in control of the script it outputs after all, including the last section linked, so I don't think this can go wrong
I actually implemented this to try it out, so I figured I'd PR it and include some other suggested changes
Thar0#7
feel free to come up with your own implementation of course
tools/mkldscript.c
Outdated
| "\t%s -s <spec-file> <segment-dir> <segment-name>\n" | ||
| "\t%s -r <spec-file> <segment-dir> <output-file>\n", |
There was a problem hiding this comment.
The tiniest bit of documentation never hurts 😃
| "\t%s -s <spec-file> <segment-dir> <segment-name>\n" | |
| "\t%s -r <spec-file> <segment-dir> <output-file>\n", | |
| "\t%s -s <spec-file> <segment-dir> <segment-name>\n" | |
| "\t\tWrite a linker script and dependencies for the specified segment\n" | |
| "\t\n" | |
| "\t%s -r <spec-file> <segment-dir> <output-file>\n" | |
| "\t\tWrite a linker script for linking all the segments together\n", |
EDIT: Thar0#7
|
|
||
| spec = util_read_whole_file(argv[1], &size); | ||
| parse_rom_spec(spec, &g_segments, &g_segmentsCount); | ||
| spec = util_read_whole_file(argv[2], &size); |
There was a problem hiding this comment.
Purely for the tool's source readability, I think it would be good to assign argv[i] to local variables with proper names so it doesn't seem so random what gets passed to functions
For example char* specFilePath = argv[2];
|
|
||
| build/$(SPEC): $(SPEC) | ||
| $(CPP) $(CPPFLAGS) $< > $@ | ||
| ifeq ($(MAKECMDGOALS),$(filter-out clean assetclean distclean setup,$(MAKECMDGOALS))) |
There was a problem hiding this comment.
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...
tools/mkldscript.c
Outdated
|
|
||
| /* end segment */ | ||
| fprintf(f, | ||
| "\t\t_%sSegmentEnd = .;\n" |
There was a problem hiding this comment.
you have probably looked at it but just in case this is the makerom segment part of new build/ldscript.txt :
_makeromSegmentRomStartTemp = _RomSize;
_makeromSegmentRomStart = _makeromSegmentRomStartTemp;
..makerom : AT(_makeromSegmentRomStart)
{
_makeromSegmentStart = .;
build/segments/makerom.o (.text)
build/segments/makerom.o (.data)
build/segments/makerom.o (.rodata)
_makeromSegmentOvlStart = .;
_makeromSegmentOvlEnd = .;
_makeromSegmentOvlSize = ABSOLUTE(_makeromSegmentOvlEnd - _makeromSegmentOvlStart);
}
_makeromSegmentRomSize = ABSOLUTE(_makeromSegmentOvlEnd - _makeromSegmentTextStart);
_RomSize += _makeromSegmentRomSize;
_makeromSegmentRomEndTemp = _RomSize;
_makeromSegmentRomEnd = _makeromSegmentRomEndTemp;
..makerom.bss (NOLOAD) :
{
build/segments/makerom.o (.bss)
}
_makeromSegmentEnd = .;
_makeromSegmentSize = ABSOLUTE(_makeromSegmentEnd - _makeromSegmentStart);
So first congrats again because it's infinitely more legible than it was previously, but I must say the indent difference on _makeromSegmentEnd looks weird, unless that's intended to match _makeromSegmentStart?
Maybe you could _makeromSegmentStart = ... before ..makerom : AT(_makeromSegmentRomStart)
and change ..makerom : AT(_makeromSegmentRomStart) to ..makerom _makeromSegmentStart : AT(_makeromSegmentRomStart)
if the linker script format allows it. (I'm not an expert)
Then _makeromSegmentStart and _makeromSegmentEnd would be on the same indentation level
(think of my OCD tharo 😢)
EDIT: see Thar0#7 unless you want to implement your own thing
|
I do appreciate the speed improvement. Actual data collected to back up claimsI tested this several times with essentially the same numbers, of course there is some natural variation but it is consistent enough. However, there are several significant problems that this introduces, some of which are avoidable, some of which seem to be inherent to partial linking.
sym_info.pyfirst_diff.py
Overall this PR seems to assume that the codebase is in a more mature and final form than it actually is: I think all of these issues are going to make it impractical while the codebase remains focussed on decompilation, matching, and even documentation for some things. I would also say that this appears to be trying to solve a problem that doesn't really exist, and while that's fine if it does not cause other regressions, I believe that the problems above are significant enough that this needs at least some adjustment (quite apart from the details of the actual implementation, which I have not yet examined in detail). |
|
Answering to elliptic, ELL-1 assumptions on segment names
😎 ELL-2 non-matching overlay segments
I think this can be solved by not depending on the ovl_ prefix to identify overlays, for example if we had a "overlay" directive in the spec a matching segment could be: and a non matching entry could be (notice no idk the specifics of where the .o files would come from then but it wouldn't be up to the spec I think EDIT: my example is flawed with respects to my idea of not linking the .ovl section unless the segment is an overlay but that could be addressed with more directives like ELL-3 losing information in build/z64.map compared to master
what is missing? Also, master: and see Thar0#7 for the corresponding excerpt in this branch There is indeed a lot less stuff (most of it moved into the individual segment linker scripts) which imo makes it heavily more readable ELL-4 multi-version support
Wouldn't that just be handled (though multiversion support has yet to happen) by different specs, ifdefs or idk what we'll end up with? The linker scripts only reflect what the spec indicates ELL-5 this breaks tools relying on build/z64.map
ah. I didn't think of that 😨
We could make the tools read from the individual segment's map files as well. I'll try to whip something up (unless someone is interested too, let me know you're doing it so we're not going two times at it) ELL-6 catastrophic for understanding bss reordering
I don't have that experience but why? I feel like at that point it's enough to match the segment anyway and we don't have less information at the segment level? ELL-boomer (jk)
😠 but the cool technology 😢 |
|
Timings for a full build / after modifying something in code / after modifying something in an actor: Graphs (careful dark mode users)
Elliptic's results are even better 😄
|
See below.
This seems considerably worse for several reasons:
Files. Most of the issues I pointed out subsequently stem at least partially from the file splits being lost from the mapfile, which removes most of the functionality. "Just read the segment ldscripts instead" is hardly a sensible option for every new script that parses the mapfile to have to do (and I would know, having written several myself).
This seems more of a flaw/redundancy in mkldscript than inherent to the current system, though: either the extra sections and alignment are not required, in which case they should not be mentioned in the first place, or you are simply sweeping the complexity under the rug into other files, creating a tree that has to be understood instead of a flat file should anything more precise be required. I'm not really sure why it helps that the main script is more readable if you end up having to troubleshoot in the messier individual ones anyway, which seems inevitable given what we've been dealing with so far.
Again, this is not the point, the point is that the files have disappeared... suppose you want to compare two mapfiles to find where something has gone, or the ordering of bss...
See above, this seems like pointless complication. I remind you that this is saving seconds, and possibly making the linker script easier to read, when surely the reason we're using the spec format in the first place is because we didn't want to deal with reading and editing a linker script...
Segments are essentially unrelated to bss reordering, bss reordering occurs at compile time and is filewise. I remind you that OoT has exactly one file that we cannot match completely... Why? Because it is the only Suppose I have some required in-function static bss. Some of the bss in bss is hard enough to deal with with the current system, this exacerbates the problem. Until we have a full understanding of bss reordering and a solution for it, we really shouldn't be making this any worse.
Okay, so let's use Python 3.11, Rust, and C++23, and Ninja. And I'm not even joking about Ninja, I'm sure it has better mechanisms for dealing with some of this stuff than I also remind you that last time I suggested taking advantage of the advanced features of make like restarting, @Roman971 rejected all of my suggestions, and I redesigned the entire system of how Fado is used from scratch to take this into account. Another problem is that this is employing advanced features of |
|
I'd love to see numbers on the RAM usage, but I don't know how to construct them. |
ELL-2 Changing the spec formatWhy would this be an issue? If we're open to moving from our spec why not alter it in the mean time
Well the idea would be relocs are entirely handled by the build system so I don't see why the "user" should have to see those in the spec (assuming you're talking about relocs)
I don't get what you mean there ELL-3 effects on readability of the split segments on the scripts and maps
We could just have a script act as a "library" for reading the various map files and collecting all the information, even putting it all together possibly. I realize it's more work to do in the PR but I think the issue can be addressed in a satisfactory manner I don't think (???) there is a way to carry the source file information all the way to the main map, but (disclaimer: possibly dumb idea ahead) we could put sections from each file into different sections. For example in code instead of we could have then the main ldscript would link in (note I didnt get this working, somehow my rushed changes ended up with ???)
It may be only personal preference then that the split scripts look better. Does it really matter anyway, we are mostly not going to look at them (though for lld support tinkering with shorter scripts sounds good). ELL-5
which is a lot when the whole thing also takes seconds! 30%-50% time saved when compiling after editing an actor! !!! 😄 ELL-6 THE BSS AAAAAHI still don't understand the stakes here. The bss doesn't move across segments (? :monkaBSS:) so you would only need the segment's own map file to figure it out, it actually seems like a shorter map would help to not get lost? ELL-new-tech
TOML > YAML 😏 (20% off your first monthly subscription by clicking this link https://toml.io/en/ ) (about why not use yaml though, a good reason is it's not in most standard libraries so a pain for tools to interact with. the current spec is also not good but at least the format is simplistic enough that regex can do "enough") fwiw I would even get rid of ido or ido recomp if someone came up with a compiler that was better behaved, supported more recent C (I want indexed array initializers), and somehow happened to OK the rom. Probably not an opinion many would share (?) but it's unlikely enough we don't need to think about it, do we
Is it? Which ones? |
Some mkldscript improvements
Currently, all object files in all segments would be linked all at once in one call to
ld. This PR adds incremental linking (withld -r) by spec segment to the build process. Now all segments as defined in the spec are linked first, and then all of those are linked together in the final link step to producezelda_ocarina_mq_dbg.elf.Relocations are generated from the incrementally linked segment files rather than individual objects, for segments prefixed with
ovl_. They no longer need to be manually written into the spec. Having a link step in between compilation and reloc generation gives us more control over the kinds of sections that fado sees, making it more robust when dealing with GCC compiled objects so the options passed to GCC need not be as restrictive.This builds marginally slower from clean, however builds that only touch a subset of files are faster.
Thanks to glank for sorting out the incremental linking integration, especially the makefile dependencies