Section ordering change was not backwards compatible #136
Comments
|
CC: @Ben10do I'm not sure about this. First of all, I'm aware of this change in behaviour, I tested the PR on my game (µCity) and realized that the binary was indeed different. I also made sure that it worked as expected, so I merged it because it didn't break anything. My game uses enough things of rgbds to be useful as a test rom, and I use floating sections for almost everything, so the fact that it worked seemed to me like a good enough reason to accept the PR. It's true that it's not 'backwards compatible', according to some definition of 'backwards compatibility'. It's also true that the real problem is that both repositories rely on something extremely fragile and undocumented, which was due to change at some point anyway because the previous system was suboptimal, as said in issue #83. This is not a real compatibility break, it's just that you relied on something that happened by accident. If you really depend on a specific order of your sections you should be using fixed sections (by both bank and address). While I don't think that the current system is perfect, it's closer to perfection than the previous one. First, you place the big sections and then the small ones to fill any holes left behind. It seems like a more sensible way of placing sections. It's like the 'rocks, pebbles and sand' story: if you first add all your sand to a jar you won't have space to add the pebbles and the rocks. If you happen to add first all your small sections you won't have space for the big ones even though they could perfectly fit if you reorganized them. Also, I'm not sure if adding a flag to the linker to make it dumber makes sense. This is not like the optimization level of GCC, the code is still the same, the sections are still placed in valid ways, and anybody using rgbds as it was designed to be used shouldn't have any issues. |
|
That being said, I don't know what your problem is. As far as I can see you are fixing all sections to banks. Maybe floating WRAM sections? Try to fix the following sections of pokered and check if it works: This goes for pokecrystal: |
|
I've fixed pokered (I think). |
|
Of course this would blow up when I'm afk 😝 I don't have too much to add to what @AntonioND said, although I definitely agree that adding an extra flag to support the old behaviour would be silly – maintaining two section assignment algorithms feels a lot like overkill to me! A slightly more in-depth explanation of the changes, if you're curious: Regardless, sorry to see that these changes has caused a blip for these disassemblies, and I'm happy to look into fixing pokecrystal (if @AntonioND or someone else hasn't already done so by the time I get there). |
|
I've created a PR for pokered (well, fixed the one I've referenced before). I'm doing the same for pokecrystal. |
|
I've just submitted a PR for pokecrystal (pret/pokecrystal#357), hope this makes up for the trouble. |
|
Oh, great, I was doing it at the same time and didn't read your message... pret/pokecrystal#358 |
|
In any case, this issue is not going to be fixed. Any other dissasembly affected by this will have to fix the affected source code. |
|
An unsatisfactory conclusion to this discussion, as projects derived from disassemblies are more severely affected. |
|
Sorry, we cannot guarantee that the linker will always have the same behaviour because that limits future optimizations and it wasn't documented anywhere that it was supposed to behave like that. A flag to go back to the previous dumb behaviour would be more code to maintain. It is not the duty of the maintainers of this repository to make sure that incorrect code works as expected. In short: If you need a fixed section, use a fixed section, not a floating section. The PRs created in this discussion are a nice gesture to the maintainers of the disassemblies referred by @Sanqui but we had no obligation to do them. |
And how would we specify "we want section B to come immediately after section A, regardless of how big they are"? Under the old behavior, this was straightforward — place them in order. That's impossible now, and making them fixed in order to be consecutive requires knowing their exact size. The old behavior made this simple. Write the sections in the order they should be, and that's it. While you may think "you can just merge them into a single section", this is not possible when multiple object files are involved, and purely impractical when the codebase is hundreds of thousands of lines long. On the other hand, there is no solution to this problem now. Fixed sections require knowing the exact size of the section that comes first, which has an obvious problem that makes the solution unusable: any single change to the section (don't think about disassemblies, think about modified versions) requires updating the address for the second section. Which in turn requires tracking where that second section is. Don't get me wrong; I understand that there are downsides to requiring sections to appear in the order they are written. But those downsides are often made up for by the allocation simplicity that this model has — if you need sections to come up in a specific order, removing this feature makes this impossible, to the point sections as a whole become unusable for the project. |
|
As I said, this didn't even consistently happen under the old allocation algorithm; the allocation order would differ depending on the section type and whether the section was bank-fixed. |
It wasn't simple, you needed to specify the order of everything, including the object files. If your Makefile or build script was a bit clever and searched for object files automatically even a simple change of name could change the order. Even splitting a file into two could break things if you aren't careful. It's not something you should rely on.
Sure, that's a problem, and the only thing I can say is to use the same section for everything that has to be contiguous. I've checked the source code of a few disassemblies, and they are a pretty good example of how not to do things (organization-wise). If you really need something to be together you should put it in a single section. If you have different files with code that should go together, include the code of each one of them in another. Just create sections called "bank 40" or whatever and include everything that should be there in the correct order. And I'm sorry that most disassemblies are affected by this, but this wouldn't have happened if they hadn't relied on this behaviour. That's simply a hack, the code is plain wrong. If you need things to be placed in a specific place you shouldn't tell the linker that it's free to place them wherever it wants by using floating sections. If you need a specific order you need to tell the linker to do it explicitly. Floating sections allow the linker to do whatever it feels like doing, which is perfectly fine if you are writing something new and you only care about using the space in an optimal way. And yes, for pure disassemblies this is less of a problem than for modified versions, for modified versions you need to modify the sizes. But still, if you create sections that represent banks you can add all the code of all involved files and be sure that it will always be together. That's what should have been done from the start to make sure that the behaviour was consistent. I'm really sorry for disassemblies, but this is their fault. I suggest they use commit 7b8d4de of rgbds until they fix their codebase. And I repeat, this was going to happen at some point but it happens that 'at some point' has been earlier than expected: #82 #83 |
|
Funnily enough, there's a really old issue in pret/pokered wondering what would happen if the linker changed its behaviour: pret/pokered#77 |
|
So, here we are with this issue. There are basically three use cases with section placement:
Case 2 requires no effort, so there's nothing to address. Case 1 is a fixed section. The issue here is case 3, since the new linker behavior makes this impossible. You say essentially that a floating section is case 2. That's fine, but how would a user do case 3 then? |
|
I'd edit my previous message, but mobile GitHub won't let me. For the record, I'm not saying that case 2 sections need to be treated as case 3. I'm simply saying that there is a true need for case 3 sections. This would easily be fulfilled if they had their own syntax, backwards incompatible or not — if, say, (The syntax is just an example; I'm not advocating for any specific syntax here.) |
|
The problem is that 3 has never been supported, you were just abusing the behaviour of the linker for some types of sections. Regarding the possibility of continuing sections, as in pret/pokered#77. What is "the section placed immediately before"? Maybe you are just feeding the linker a bunch of object files, which one is the first one? What if you sort them alphabetically before feeding them to the linker, one of them changes the name and the order is different? What if you define a second section in a bank and then want to continue the first one? Maybe joining sections with the same name could be a fix, but it is a hacky fix (or it seems to me quite hacky, right now it is useful to disallow that to detect mistakes). There are more problems. For example, relative jumps are only allowed within sections, but the continuation system wouldn't let you use them between different parts of sections. What you are proposing for 3, after taking a look at the disassemblies, should be done like this with the current linker: The alternative is to allow to continue sections, which is problematic as explained above. |
|
There is exactly one reason why that's not a full solution — multiple
object files. Those are used to prevent unnecessary recompilation, but
sometimes a bank is split across multiple files. The only way of preserving
a stable location for sections is to use cases 1 or 3, and case 1 is
problematic for reasons already mentioned.
|
|
With the current linker, if 2 chunks of code need to be together they belong in the same section, there's no discussion there. If you don't want to recompile everything all the time you can have different sections for different banks in different object files and then link them together at the end. But all the code that needs to be together needs to be compiled in the same section. Something like "banks_10_to_40.asm" and have all the code of those banks there. Now, regarding 3. I think that the If you disassemblers needed that extra functionality, you could have requested it instead of misusing the current behaviour. I'm not going back to the previous one because that sets the precedent that the toolchain needs to support whatever crazy tricks are being done by anyone. I know that in this case there's a lot of people affected. That's because they all copied the same flawed system, and that's not the fault of the new section placement code. If there is any proposal for how to do 3, perfect. But it has to be consistent. "Continue the section before this one" is not a consistent behaviour because it depends on too many factors. It's not just the order of the sections inside an object file, but the order of the object files when they are linked together. That's simply not good enough. |
|
A completely new feature to replace the "misused" one would be perfectly
fine — it's possible to adjust the code to use the new feature. The main
reason for the complaint is that support for case 3 is completely gone.
That being said, one of the use cases comes from having separate files
(assembled separately and linked) like this:
one.asm
```
SECTION "1A", ROMX, BANK[1]
; code
SECTION "2A", ROMX, BANK[2]
; code
```
two.asm
```
SECTION "1B", ROMX, BANK[1]
; code
SECTION "2B", ROMX, BANK[2]
; code
```
(Note that the order in which the files are passed to rgblink is entirely
controllable by the Makefile, and thus not an issue, in this case at least.)
Under the previous behavior, sections 1A and 1B would have been contiguous
and in that order, and likewise for 2A and 2B; also, both 1A and 2A would
have been located at $4000, being the first sections of their respective
banks.
How would you implement that, considering the following conditions?
1. The files, one.asm and two.asm, cannot be merged and must be linked
2. The sizes of 1A and 2A are unknown (prior to compilation)
3. The sections have to appear in their "usual" locations (1A at 01:4000, 1B
immediately after, 2A at 02:4000 and 2B immediately after)
I'm interested in your suggestions about how to solve this case.
…On 15 Mar 2017 10:29, "AntonioND" ***@***.***> wrote:
With the current linker, if 2 chunks of code need to be together they
belong in the same section, there's no discussion there. If you don't want
to recompile everything all the time you can have different sections for
different banks in different object files and then link them together at
the end. But all the code that needs to be together needs to be compiled in
the same section.
Something like "banks_10_to_40.asm" and have all the code of those banks
there.
Now, regarding 3.
I think that the ALIGN keyword was something long overdue, and the change
of behaviour to fill holes was necessary. That's not going to change: this
toolchain is not only used by disassemblers but by developers, and this is
something that developers (like me) needed a long time ago.
If you disassemblers needed that extra functionality, you could have
requested it instead of misusing the current behaviour. I'm not going back
to the previous one because that sets the precedent that the toolchain
needs to support whatever crazy tricks are being done by anyone. I know
that in this case there's a lot of people affected. That's because they all
copied the same flawed system, and that's not the fault of the new section
placement code.
If there is any proposal for how to do 3, perfect. But it has to be
consistent. "Continue the section before this one" is not a consistent
behaviour because it depends on too many factors. It's not just the order
of the sections inside an object file, but the order of the object files
when they are linked together. That's simply not good enough.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#136 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AF2FfEs-Gem9PBOsmKDDKtG79ivLUvSjks5rl-ekgaJpZM4Mc7RW>
.
|
|
First of all. This is not a regular game. You are trying to assemble code into a reference ROM. That is good enough reason to use a code similar to what I wrote before. I repeat, what you are proposing is simply not supported and it was never supported. That being said, something like this could be implemented: Not sure about the interactions with |
|
Finally getting around checking on this. I agree that the old behavior shouldn't have been relied on in the first place, and I didn't realize it was inconsistent between section types either. Despite that, backwards compatibility would have been nice. I will respect the maintainer's choice however. I want to extend a thank you to @AntonioND and @Ben10do for stepping forward and voluntarily correcting the issues in the |
|
Reference ROMs are modified, and it in the spirit of allowing and simplifying those modifications that I'm making these suggestions. At this point I have to ask, how about a manifest file? A separate file listing sections and their intended locations, such as: Of course, any format is acceptable. EDIT: needless to say, the linker would be free to place unmentioned floating sections anywhere. That's all of them if no manifest is used. |
|
That would involve big changes in the linker, starting with a parser of the manifiest file, which would only be useful for this corner case. A I'll give others a chance to say what they think it's a better option, anyway. New ideas are welcome. |
|
The issue with a CONTINUES keyword is that it splits the section management
into multiple files. (In the example, two.asm references one.asm directly.)
From having spent a great amount of hours managing the space and
allocations in a project, I can tell you one thing — centralizing this
configuration (such as with a manifest file) makes things simpler, by far.
(You might be thinking that the usual model splits things too. We used
macros and constants to avoid this, and pointed all bank numbers to a
single file; the order of the file matched the order of the source files,
which ensured that everything worked as intended. A manifest file is
essentially what that file intended to be, by abusing the linker.)
In essence, the problematic issue here is that what we need is a way of
specifying the order of sections, via some mechanism that doesn't become a
nightmare when we need to change that order. Stating the order explicitly
in the section header doesn't sound like the way to go, unless there is
some strange macro-based way I'm missing of centralizing this configuration.
…On 15 Mar 2017 11:16, "AntonioND" ***@***.***> wrote:
That would involve big changes in the linker, starting with a parser of
the manifiest file, that would only be useful for this corner case. A
CONTINUES keyword would basically join two sections inside the linker,
making it really easy to implement (we would only need to modify a bit the
parser of the assembler and the format of the object file).
I'll let others a chance to say what they think it's a better option,
anyway. New ideas are welcome.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#136 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AF2FfCluCk40N_XVIW2knNJpJKC_6RXqks5rl_LLgaJpZM4Mc7RW>
.
|
|
Both sound like valid approaches to me; while parsing the manifest file would be more work, it sounds quite a bit more structured to me - perhaps this would be a great thing to add if we do rewrite the linker. Equally, a Also, it'd probably be a good idea to create a new issue for the feature request (including these proposed solutions), and leave this bug report closed. |
|
I'll be happy to help with the implementation if you need a hand, although I'm hardly available during the week nowadays. Weekends should be fine. Do you want to make the feature request issue yourself, or should we make it? |
|
I really don't mind; if you're happy to make it, you seem to have the best grasp of the reasons why this is needed 😉 |
|
The problem with the manifiest is that it is essentially creating a parallel system of linking sections. What happens if you specify 2 different placements for a region in both the source code and the manifiest? That cannot happen with the CONTINUE keyword. I'm generally in favour of small changes, this one seems to be a really big change for this use case. And I'm specially against adding new parsers because the one of rgbasm is broken enough for the whole codebase. But yeah, let's discuss this in a new issue. |
|
Done. |
In 1b05c43, section ordering was changed:
I am assuming the old algorithm allocated sections linearly in the order they appear in the source.
This breaks both https://github.com/pret/pokered and https://github.com/pret/pokecrystal (issue pret/pokecrystal#356) as they relied on the previous behavior. (The ROMs built no longer match the originals.)
In my opinion, the old behavior should be brought back and the new behavior should be placed under a flag.
Thanks to @FredrIQ (and @aphirst) for noticing.
The text was updated successfully, but these errors were encountered: