Is this a bug in ARMTHUMBINSTRUCTIONS.sinc ?

[pcjc2]

I was looking at commits to the ARM architecture, and came across a patch:

40c8aa6

@@ -2396,7 +2396,7 @@ define pcodeop ExclusiveAccess;
:ldrsb^ItCond^".w" Rt1215,PcrelOffset12 is TMode=1 & ItCond & (op8=0xf9 & thc0506=0 & thc0404=1 & sop0003=15; Rt1215) & PcrelOffset12
{
build ItCond;
tmp:1 = *PcrelOffset12;
tmp:1 = PcrelOffset12:1;
Rt1215 = sext(tmp);
}

@@ -2437,7 +2437,7 @@ define pcodeop ExclusiveAccess;
{
build ItCond;
build PcrelOffset12;
tmp:2 = *PcrelOffset12;
tmp:2 = PcrelOffset12:2;
Rt1215 = sext(tmp);
}

I noticed that one block has the (instruction?) build PcrelOffset12; the other does not.

I don't actually know what the meaning of this line is, note that most? blocks referencing PcrelOffset12 have the line build PcrelOffset12;

I'm curious to know if this is an omission / bug / gap in my understanding.

Regards,

Peter

[GhidorahRex]

The build directive is used to tell sleigh when a sub-constructor's pcode should be executed. The sub-constructors are normally executed immediately before the parent constructor's code, but if ordering is important, then the build directive can control that process. For example, with the COND or ItCond sub-constructors, we want those to execute before any other code, so the first pcode instruction is always build COND or build ItCond.

Another example can be seen with the thumb S-bit check which determines whether the flags should be written or not. The pcode for updating the flags is found within the sub-constructor, but we don't want to update the flags until after all the other pcode instructions have been executed. The build thSBIT_CZNO directive is used to tell sleigh that we want that pcode to occur after all the other pcode in the instruction.

All that being said, because sub-constructor pcode normally occurs at the beginning of the subconstructor, a majority of the build directives aren't needed - only if there are side-effects. In this case, thePcrelOffset12 constructor does not have any side-effects, so it's not a bug. It would probably be good practice though.

When reviewing the code to answer your question, I noticed that there is a bug with the build directives in ARMTHUMBinstructions.sinc. The thshift2 sub-constructor has several variants with a side-effect of writing to the shift_carry shadow register which we use to update the CY bit. If these instructions were to occur in an IT block, the shift_carry register might be erroneously written, when it shouldn't be.