I'm trying to implement some error checking for assembler code for the AVR target (e.g. issues 32007, 32109 and 32261).  After stepping through the compiler code a couple of times it seems as if there isn't a proper assembler constraint check build into the GAS parser in fpc. 

The constraint checking I'm referring to is for example: sbiw R18, 1  // R18 not a legal register - sbiw only operates on registers R24, R26, R28 & R30 cp R18, 20   // 20 not a legal register, cp operates on 2 registers only breq l1      // check that -64 < l1 offset < 64 and raise error if in assembler block

It seems as if some basic checking is being done in aasmcpu.pas procedure finalizeavrcode, but this operates on instructions generated both by assembler blocks and regular code.  I've started adding some checks to finalizeavrcode procedure, but it seems clunky to me.  Is there a better location in the compiler to implement some assembler consistency checks?

Also, how do I manage/add new compiler error messages for these checks?

Christo wrote

I'm trying to implement some error checking for assembler code for the AVR target (e.g. issues 32007, 32109 and 32261).  After stepping through the compiler code a couple of times it seems as if there isn't a proper assembler constraint check build into the GAS parser in fpc.

No. This is indeed left to the assembler.

This method is used to fix up code generated by the compiler.

but this operates on instructions generated both by assembler blocks and regular code.  I've started adding some checks to finalizeavrcode procedure, but it seems clunky to me.  Is there a better location in the compiler to implement some assembler consistency checks?

fpc/compiler/avr/raavar.pas:TAVRInstruction.ConcatInstruction

Also, how do I manage/add new compiler error messages for these checks?

Add them to fpc/compiler/errore.msg, run make msg

Attached find a patch for review which checks consistency of operands for the instructions NOP, CP and SBIW. I haven't added new error messages yet, I try to re-use messages if they seem appropriate. It appear as if it would be easier in the long run to create some kind of generic operand constraint array and iterate over this?

At some point (much later) I also want to add range checks for branch type instructions (rjmp, breq etc) to make sure that the branch distance is within reach of the instruction.  I suppose this check will have to be implemented in finalizeavrcode since here the offsets of instructions seem to be calculated? 

constraint array and iterate over this?

Well, yes, the other architectures with inline assembler checking (i.e. x86) use the tables of the internal assembler for this.

At some point (much later) I also want to add range checks for branch type instructions (rjmp, breq etc) to make sure that the branch distance is within reach of the instruction.

This should be checked by the assembler reader.

I suppose this check will have to be implemented in finalizeavrcode since here the offsets of instructions seem to be calculated? 

finalizeavrcode is used only to fix code generated by the compiler. It should not be used for error checking imo.

Attached please find a patch for implementing some assembler validity checks for review.  I'm sure I've missed some tricks to optimize and reduce the code, but at least I would appreciate a check to see whether the logic of the different checks is sound. Particularly the bit that checks whether address registers are used correctly.

To me it looks good. I propose to commit if it breaks no existing code, time will tell if the checks are ok :)

Florian Klämpfl wrote

To me it looks good. I propose to commit if it breaks no existing code, time will tell if the checks are ok :)

Great news, do I have to create a bug report with the patch? I also have a test program with pass/fail code snippets to test the logic - should I chop it up into single instruction test cases for automated testing? How to submit test cases?

Just make a zip of the test programs. If they should fail during compilation, add a { %fail } comment as the first line, this tells the test runner, that the program must cause a compilation failure. If the test is useless, but should compile, add a { %norun }

Florian Klämpfl wrote

Just make a zip of the test programs. If they should fail during compilation, add a { %fail } comment as the first line, this tells the test runner, that the program must cause a compilation failure. If the test is useless, but should compile, add a { %norun }

Attached please find updated code for ConcatInstruction (raavr.patch) and some test cases in tests.zip. The one test, tbs614.pp contains a list of all instructions with what I consider permutations of valid parameters, all of which I think should successfully compile. The other 3 tests are for bug reports I've submitted. 

One issue I've noticed is that in rautils.pas TOprRec.val is defined as word for AVR.  This creates a problem with interpreting large constants or negative constants.  This is probably a corner case because most of the time offsets in code will be calculated by the linker, but it in principle a user could specify a jmp with a user specified constant instead of referencing a label. The consequence of this is that negative numbers are incorrectly casted as absolute values (bug 32946) and also very large valid constants are silently truncated. If it is accepted that user specified constants are allowed for these cases, the val field of TOprRec should be expanded to a larger type, probably a longint.

Another issue I've noticed is the case:   ldd r20, z+0 where the instruction is internally represented as ldd r20, z which is of course invalid syntax. I haven't had time to dig too deeply into this case since this seems like a very unlikely scenario to crop up (but it is a valid instruction according to the AVR instruction manual).

The webtbf32261.pp test check that a constant is not misinterpreted as a register, while webtbs29758.pp check that accessing a procedure parameter via type cast passes the consistency check.

Christo wrote

Another issue I've noticed is the case:   ldd r20, z+0 where the instruction is internally represented as ldd r20, z which is of course invalid syntax. I haven't had time to dig too deeply into this case since this seems like a very unlikely scenario to crop up (but it is a valid instruction according to the AVR instruction manual).

I've got a possible fix for this case, see patch attached to bug #33086.

Thank you, I committed your patch in r38286 with indentions as FPC uses them.