Edigen should be in the Maven Cental Repository.

emuStudio and related projects were moved to Gradle already. Build code is easier to read, simpler.

This should be possible:

instruction = prefixed | 0xEF;
optionalPrefix = 0xFF instruction; 

Example:

root instruction;

instruction = \"a\": 0x00 other | \"b\": 0x10;
other = 0x11 ref16;
ref16 = ref16: ref16(16);

%%

"%s\" = instruction;

generates maxInstructionBytes = 3 instead of 4.

Currently it is possible to write such input file that more than one variant of a rule can match. This can cause unintended behavior of the generated instruction decoder.

It would be useful to detect such cases and stop the generator, printing an error message.

For example, instruction

lxi SP, 0E3ABh

is dissassembled as

lxi SP, -01C55

Unfortunatelly, this is probably caused by BigInteger.toString() inside Disassembler.edt method format().

I think it will be possible to use here RadixUtils.convertToRadix() from emuLib.

Instructions are read one unit at a time. Currently, this unit is hard-coded to one byte. There should be an option to change it to short (2 bytes) or int (4 bytes).

There should be support of multiline comments, also multiple one-line comment formats could be supported:

• multiline: /* ... */
• oneline: # comment (already supported)
• oneline: // comment
• oneline: ; comment

• GenerateMaxInstructionBytes is not working properly
• decoder doesnt include just read bytes in the instruction image (it puts there full max instruction bytes), thus next instruction position is always + max instruction bytes instead of actual size

After merging #35, there is a problem that typos may become unnoticed. For example, in

instr = add(8);
addd = ...;

the addd = ...; rule definition should ideally throw an error if the addd rule is not used anywhere.

See emustudio/emuStudio#141

Currently, a rule can have multiple names, e.g.:

instruction, data = line(5) 111;

However I don't really see a point why. I think one rule should have unique name and it should be an identifier of the rule. The current situation complicates identifying rule in code, e.g.:

    /**
     * Returns a human-readable label of this rule - a name or a list of names
     * separated by commas.
     * @return the label
     */
    public String getLabel() {
        Iterator<String> nameIterator = names.iterator();
        StringBuilder result = new StringBuilder();
        
        while (nameIterator.hasNext()) {
            result.append(nameIterator.next());
            
            if (nameIterator.hasNext())
                result.append(", ");
        }
        
        return result.toString();
    }

In my opinion this is unnecessary. @sulir do you have some use cases why this feature might be useful? Thanks!

For example:

root instruction;

instruction =
  "nop": 0000 0000 |
  "arg %X": 10000 0000 arg ;

arg = arg: arg(8);
%%

"%s" = instruction arg;
"%X" = arg;  // unreachable

Plain arg rule is unreachable, because all instruction variants return, so instruction is always present too.

When working on SSEM CPU, I ran into the following exception:

Exception in thread "AWT-EventQueue-0" java.lang.ClassCastException: java.lang.Integer cannot be cast to java.lang.Short
    at net.sf.emustudio.ssem.DecoderImpl.read(DecoderImpl.java:75)
    at net.sf.emustudio.ssem.DecoderImpl.instruction(DecoderImpl.java:119)
    at net.sf.emustudio.ssem.DecoderImpl.decode(DecoderImpl.java:59)
    at net.sf.emustudio.ssem.DisassemblerImpl.getNextInstructionPosition(DisassemblerImpl.java:125)
    at emustudio.gui.debugTable.InteractiveDisassembler.updateCache(InteractiveDisassembler.java:97)
    at emustudio.gui.debugTable.InteractiveDisassembler.rowToLocation(InteractiveDisassembler.java:187)
    at emustudio.gui.debugTable.DebugTableModel.getValueAt(DebugTableModel.java:113)
    at javax.swing.JTable.getValueAt(JTable.java:2717)
    at javax.swing.JTable.prepareRenderer(JTable.java:5706)
    ...

The code points at Decoder.edt template, where I found the following snippet:

private byte read(int start, int length) {
        ...
        if (start + length > 8 * bytesRead) {
            instructionBytes[bytesRead++] = ((Short) memory.read(memoryPosition++)).byteValue();
        }
       ...

The MemoryContext.read() returns generic type. It can be anything. We can agree that in order to use decoder, there should be a requirement that the context type should at least extend Number, which has the desired byteValue method. It is a superclass of all numbers (Integer, Long, Short, Double, Float, Byte), so it should be fine for future as well.

Specifying the Big / Little Endian for disassembler in the command line arguments seemed as a good idea at first. But the endianness is an integral property of a processor, not something which can be accidentally changed by forgetting to use a command line switch during a compilation.

The cache should work like this: Every time an instruction is decoded, it is placed into the cache of some capacity. Next time, if the supplied bytes are exactly the same, the decoded instruction is returned directly from the cache, without performing the decoding process.

There is one problem - the size of the instruction is unknown before decoding. So the cache should be a combination of a tree and hash-maps.

Something similar could be also done for the disassembler.

If the insertion and selection were fast enough, this would significantly improve the emulator speed.

Travis CI now supports SNG badges.

Logging information about the generation progress could be useful. There exist libraries like Logback / SLF4J for this.

Unnecessary read of unit

In GenerateMethodsVisitor, when the unit was read in a method when processing a Mask, with given start and length, it is not necessary to read it again in nested switch.

unit = read(start + 0, 5);

switch (unit & 0x1f) {
        case 0x10:
            ...

        default:
            unit = read(start + 0, 5);  // <-- unnecessary
            switch (unit & 0x1c) {
                case 0x08:
               ...
...        

Unnecessary read of unit no.2

In GenerateMethodsVisitor, when the mask is zero, there won't be any pattern as the child of the mask (in any depth). If there was, current code will be broken anyway, because Pattern will generate case and default switch branches, while the switch itself is defined in the parent mask only if the mask is not zero. Therefore it is not necessary to read unit. Variant returning subrules will use readBytes anyway.

    private void line(int start) throws InvalidInstructionException {
        unit = read(start + 0, 5);  // <-- unnecessary
        instruction.add(LINE, readBytes(start + 0, 5));
    }

Do not use break under default branches

It is unnecessary to call break; in the default: branch, since it's always the last branch.

                                    }
                                    break;
                                }
                                break;
                            }
                            break;
                        }
                        break;
                    }
                    break;
                }
                break;
            }
            break;

I have doubts about readBytes

As I traverse the code - from decoder to disassembler, I'm starting to think that byte[] array - of instruction "bits" is not necessary. I'm talking about:

public class DecodedInstruction {
    public void add(int key, byte[] bits) {   // <-- this one
...

With this it is connected method readBytes in Decoder, which is very complicated and hard to understand. If you imagine this method is often called only for few bits, it can be pretty expensive call.

Anyway, in disassembler the "bits" are somehow presented - either as integers, floats, doubles, or strings. If we have bits which have more than 4 bytes, we cannot present it as integer (but we allow it). For doubles I agree 8 bytes should be supported, if we want to emulate some modern CPUs. But actually I doubt it. I think emuStudio will always be emulating older machines or some esoteric machines. Therefore I suggest to stick with 4 bytes as maximum (which will be also hard limit for instruction size, because root rule mask has usually size of the whole instruction). Also, unit size is 4 bytes.

Maximum instruction size on modern processors is only few bytes

It is unnecessary to store 1024 bytes per instruction (field instructionBytes). Even modern CPUs on x86 have instruction size of maximum 120 bits which is 15 bytes. Therefore it will be better if we read instructionBytes ahead. For preserving "generality" we should first find out instruction maximum size by some new visitor, which will store it in a constant. Then, using MemoryContext.read(memoryPosition, count) we can try to read max bytes. If there is less bytes available, the method will return only those bytes which are available and will not throw.

Remove "+ 0"

It doesn't look good when the code has + 0 - we know that it's a zero so we don't need to add it. E.g.:

private void instruction(int start) throws InvalidInstructionException {
        unit = readBits(start + 0, 32); // <-- here
        
        switch (unit & 0x00070000) {
        case 0x00000000:
            ...
            line(start + 0);  // <-- here
            ...

Subrules are associated with nonexistent main rules after the second syntax tree pass. This can cause NullPointerException.

There are many situations where the input file contains syntactically correct, but semantically incorrect information. Possible reactions are (from the best to the worst):

• print a meaningful error message containing details about the error
• generate syntactically incorrect Java file
• print a typical message about an unhandled exception ("Exception in thread...")
• generate syntactically correct Java file which will produce unexpected results upon execution

The first two are acceptable, the others are not.

Semantic errors in a decoder include:

• multiple rules with the same name [1]
• subrule refers to a nonexistent rule [2]
• variant returns a subrule not present in the variant [3]

In a disassembler:

• value refers to a nonexistent rule [4]
• multiple formats contain the same values [5]
• value refers to a rule which can never return a value [6]

Of course, the lists are not complete.

Edigen should allow formatting in rule names, e.g.:

  "ld %s, %s":   01 0 r_bcde(2) r(3)              |  # x=1, y=0 r_bcde, z=r

the specification file can be then so much shorter and cleaner!

emuStudio is also using GPL v3, and since edigen doest contain any parts which do not include source code, it can be easily migrated.

Some of the semantic error messages would be more useful if they contained line numbers.

At least the Visitors should be tested.

Sometimes it is handy to recover from InvalidInstructionException while decoding by trying another rule - e.g. if the bytes do not represent an instruction, they represent data (and we want to show it in dissasembler).

Proposal 1

Currently, the first rule in the file is the only root rule. If this rule fails, there's no possibility to try other path. One solution would be to have multiple root rules (either explicitly specified - or taken as the first rules appearing in all lines in the dissasembler part) and try apply from the first to the last. For example:

...
%%

"%s %d" = instruction line(shift_left, shift_left, shift_left, bit_reverse, absolute) ignore8 ignore16;
"%s" = instruction ignore8 ignore16;
"%s %d" = number num(bit_reverse);

In this code the rules instruction, number are root rules and will be tried in this order.

Proposal 2

Another solution would be being able to capture the unmatched input in the only root rule, for example as:

instruction =  "JMP": line(5)     ignore8(8) 000 ignore16(16) |
               "JPR": line(5)     ignore8(8) 100 ignore16(16) |
               ... |
               <otherwise> "DATA": data(32)

We can debate how the <otherwise> part should be named. This part would be optional.

Why the <otherwise> is this actually needed is because of possible ambiguities with instructions (e.g. pattern 000...0 is ambiguous). I don't really want to explicitly specify all possible values except the ambiguous ones - this is what I expect from the <otherwise> part.

@sulir what do you think about it?

In comments it is not possible to use unicode characters (e.g. chars with punctuation) like č, etc.

All instructions using numbers, both 8 bit or 16 bit, have not disassembled the low-order byte. For example:

cpi 55h

is disassembled as

cpi 00

However, the opcode is correct (FE 55)

Documentation about Edigen usage should be added to README or Wiki pages.

The disassembler changed from SimpleDisassembler to AbstractDisassembler and Decoder is already included.

Also please consider if it would be a good step to generalize disassembler and move the code (of a template) into emuLib directly. In fact, only static tables of disassembler formats and values need really to be generated, everything else can be generalized. These tables can be passed either in constructor, or through setters (if we would like to allow change format at runtime) to the disassembler.

Rules like src_mem = mem: mem(16); are a little bit redundant. If there is a definition similar to

arith_operands =
    001 dst_reg(3) 01 src_mem(16);

and the rule src_mem does not exist, then instead of printing an error, the rule should be implicitly constructed according to the former example.

If a variant is a direct child of a rule, it is lost after the variant-moving transformation.

The release contains just one bugfix (#67) and migration to GPL v3 (#66)

There exists an another possible style of generated decoder code, which uses the return statement after a variant is successfully recognized and throws an exception at the end of each rule. The default branches are not used at all.

This style should allow less strict ambiguity detection and the performance could be improved. However, it would bring some new issues which must be concerned, e.g. the instruction length recognition would be more difficult.

In addition to currently supported big-endian and little-endian, custom strategies like "bit reverse" should be supported.

One option is to use formatting character modifiers: e.g., %+d for big-endian, %*d for bit-reverse.