Parse signed dwords properly #10
Conversation
|
Shouldn't it be 32 instead of 31? This affects all dwords, not just strrefs, so we need to be careful. Eg. some of the flag fields use bits up to 0x80000000 (2^31), which is now broken. Even with 32 and it's even worse, since the lower bits could be set too. So I guess we need a compromise and I propose we just treat specially only everything higher than 2^32-11. |
|
Back when we discussed this on the forum you've suggested checking that a dword is outside of range of 2^31-1 in order to process it in a special manner. Can't reference that discussion at the moment because G3 is down. But I'll make a patch as per your suggestion here. BTW, I suppose "-11" in "2^32-11" is a typo, right? |
|
I did, but I was wrong. If we want to minimise overlap, -7 is even better, but I can't really think of good examples. Do as you see fit. |
|
Maybe there's another way to do this... It's more tedious, but we could specify in the format's structure which values should be interpreted as signed, as in READ_SHORT / READ_SSHORT as provided by WeiDU. For example, if it's an item slot in the creature file, those should be interpreted as signed since "-1" is a default value that means "no item". I don't have a better idea at the moment. And I doubt I could make a better assumption on the issue described above. We can also ask Argent77 or just dig into NI's code to see how it interprets values. |
|
Is not a better option to add a flag to read_dword()/read_word() and eventually define another type or subtype, e.g. INT32, whatever? Even more if it's for strrefs, since they already have their own type. |
|
Oh, right, we do already have a strref type. I agree then, just read them differently via another param to read_dword(). Writing doesn't need to change. |
|
How exactly do you propose to employ said parameter? When printme() is run, we iterate over the format's fields and call appropriate read_* methods in accordance with their types. Passing an argument to read_dword() within the scope of current implementation suggests extending format's definition with extra information on whether the value should be interpreted as signed or not (as I proposed in my previous comment). In this case, I'd say that introducing a new type appears to be more robust and less of a workaround. |
|
Isn't it enough to pass the flag at infinity/format.py:292 ? |
|
Strrefs aren't the only thing that should be interpreted as signed integer. There are also parameters 1&2 for various effects, item slots in CRE files and so on. |
|
Ah yes, for that a new type is needed, but it won't obviate the need for the strref change. |
|
I've pushed some changes. Let's leave it like that for now. I'll investigate the subject of the new type later and get back to you with a proper patch. |
|
Ok, thanks. I'll cherrypick this, so the first two can be squashed. |
|
Done. I'll close this and open an issue as a reminder: #12 |
|
@critto-bg : for some reason I had not seen your READ_SHORT / READ_SSHORT comment when I posted mine. Yes, I think that's the correct way to solve it - to have methods that read signed values (whether with a flag or separate methods) and some discriminant in the field definition - this might be done with a specific type (my preference) or a flag in the field def. The reason I did not bother to define them in iesh so far was that iesdp mostly does not specify signedness. |
|
Sorry for the double post, mixed up my accounts. How shall we name these types? Any suggestions? |
|
I'd go for SIGNED_DWORD and SIGNED_WORD/SIGNED_BYTE if needed. |
|
To be honest, in my other binary parser I went for INT32 / UINT32 / INT32BE etc. precisely to avoid this dilemma :) |
No description provided.