Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Assembler - Simplify AssemblyRegister struct #113

Open
Symbai opened this issue Jul 26, 2020 · 2 comments
Open

Assembler - Simplify AssemblyRegister struct #113

Symbai opened this issue Jul 26, 2020 · 2 comments

Comments

@Symbai
Copy link
Contributor

Symbai commented Jul 26, 2020

The more I work with the assembler, the more I noticed that it doesn't work very well in combination of the disassembler. For example the code below is unnecessarily bloated. This is just a very short demo, in real scenarios its even much more code.

decoder.Decode(out var instruction);

if (instruction.MemoryBase.IsGPR64()) {
	c.push(new AssemblerRegister64(instruction.MemoryBase));
	c.push(new AssemblerRegister64(instruction.MemoryIndex));
}
else {
	c.push(new AssemblerRegister32(instruction.MemoryBase));
	c.push(new AssemblerRegister32(instruction.MemoryIndex));
}

Why cannot we have something like?

c.push(instruction.MemoryBase); 
c.push(instruction.MemoryIndex); 

or in case it must be a struct

c.push(new AssemblyRegister(instruction.MemoryBase));
c.push(new AssemblyRegister(instruction.MemoryIndex));

Because if we take a look at the source code:
https://github.com/0xd4d/iced/blob/312240c36de035b790b31d18f6e2caa3aa149bd3/src/csharp/Intel/Iced/Intel/Assembler/AssemblerRegister.cs#L194-L198
, the AssemblerRegister structs are very likely all the same and therefore can be simplified/unified and check in its constructor whether its 16 / 32 or 64bit:

byte Bitness;
internal AssemblerRegister(Register value) {
			if (value.IsGPR64())
			  Bitness = 64;
			else if (value.IsGPR32())
			  Bitness = 32;
			else
			  Bitness = 16;
			Value = value;
			Flags = AssemblerOperandFlags.None;
		} 
@0xd4d
Copy link
Contributor

0xd4d commented Jul 26, 2020

All reg kinds have their own struct so all mnemonic methods are smaller and easier to generate. The compiler can also check at compile time if it's a valid operand.

@Symbai
Copy link
Contributor Author

Symbai commented Aug 15, 2020

I've worked with the assembler for quite a while now and I've ran into this issue described in the first post more and more. It bugs me to write the same code 2-3x only because I have to cover AssemblerRegister16, AssemblerRegister32 and AssemblerRegister64. When I compile no errors are raised but only in runtime it throws an error when I try to create an instruction with a wrong bitness register.

Because of that it makes absolutely no sense to me to have 4 different AssemblerRegister structs. In real world applications it forces developers to create functions for each of them. It creates lots of duplicate code. I'm not sure if its worth the "smaller mnemonic methods". If MY (or any other developer using Iced) methods are getting bigger. Because Iced source code is mainly generated it would be very less afford to create a unified AssemblerRegister class.

If you ever write code which has to cover all bitness on the fly using Iced's Assembler you get a feeling of how awful the current AssemblerRegister approach is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant