search

Kingcom / armips

An assembler for various ARM and MIPS platforms. Builds available at http://buildbot.orphis.net/armips/
MIT License
359 stars 77 forks source link

Fix initial negative headersizes #150

Closed Prof9 closed 5 years ago

Prof9 commented 5 years ago

I'm using ARMIPS to generate some files where the game skips the first 4 bytes of the file when reading relative addresses (e.g. relative address 4 actually points to absolute address 8). This can be achieved by setting a headersize of -4 in ARMIPS. However, ARMIPS currently throws an error when you try to do this at the start of the file, because for some reason it checks whether 0-4 is a valid address, even though it just seeks to 0 anyway.

A workaround for this would be to start off with a headersize 0, make sure at least 4 bytes are written, and then setting .headersize -4. But I don't see a reason why this shouldn't be possible from the start as well.

This PR removes the physicalAddress + headerSize < 0 check in GenericAssemblerFile::seekPhysical so that initial negative header sizes can work again. I think this error is being thrown erroneously in this situation.

A related PR for this is https://github.com/Kingcom/armips/pull/90 which removed support for initial header sizes by making ARMIPS seek to the start of a file upon opening/creating it, but this seems like more of a consequence rather than the intent, since it doesn't touch the seekPhysical() function at all. Rather, the check was added in https://github.com/Kingcom/armips/commit/a19efeff0b99eae59b0afdaf50222f9edf9c35cf which intended to add support for negative headersizes.

sp1187 commented 5 years ago

The problem with initial header sizes is that the virtual address will be nonsensical (i.e. negative), and the check is intended to protect against that ever occuring. Running seekPhysical at the beginning is just a quick way to perform that check at the start without repeating code, since it was already checked when seeking.

Prof9 commented 5 years ago

I disagree that a negative virtual address is nonsensical. It is what it is: a negative virtual address; an address that exists before the memory location your file will be loaded to. Suppose the file is being loaded to 0x2000200, then a virtual address of -4 resolves to 0x20001FC; not nonsensical at all. Sure, it may usually not be what you want, but it has its uses. Given that virtual addresses (and even physical addresses, which are definitely nonsensical when negative) are signed in ARMIPS, it shouldn't lead to any internal problems, as far as I'm aware.

Prof9 commented 5 years ago

How about this? I've changed the errors thrown when seeking to a negative virtual address to warnings instead. This makes clear the intent that negative virtual addresses are discouraged, but still allows the option to do it if you know what you're doing.

sp1187 commented 5 years ago

As there currently are cases where discouraged or unexpected features are allowed with warnings (that can optionally be turned to errors), such as non-aligned Thumb instructions and integer division by zero, making this a warning should be fine.

Do you think you could rebase this PR to a single commit?

sp1187 commented 5 years ago

Regarding the new commit message, don't you mean "non-negative" instead of "nonzero" in "as as long as the physical address is still nonzero"?

Prof9 commented 5 years ago

Whoops. Fixed.