On 8/27/19 3:32 AM, Peter Maydell wrote: >> +static bool trans_HVC(DisasContext *s, arg_HVC *a) >> +{ >> + if (!ENABLE_ARCH_7 || IS_USER(s) || arm_dc_feature(s, ARM_FEATURE_M)) { >> + return false; >> + } >> + gen_hvc(s, a->imm); >> + return true; >> +} > > I was wondering about for these trans_ functions the > difference between returning 'false' and calling > unallocated_encoding() and then returning 'true'. > If I understand the decodetree right this will only > make a difference when the pattern is inside a {} group.
Correct. > So for instance here we have > > { > [...] > { > HVC 1111 0111 1110 .... 1000 .... .... .... \ > &i imm=%imm16_16_0 > CPS 1111 0011 1010 1111 1000 0 imod:2 M:1 A:1 I:1 F:1 mode:5 \ > &cps > UDF 1111 0111 1111 ---- 1010 ---- ---- ---- > } > B_cond_thumb 1111 0. cond:4 ...... 10.0 ............ &ci imm=%imm21 > } > > which means that if the HVC returns 'false' we'll end up > trying the insn as a B_cond_thumb. Correct. > In this case the > trans function for the B_cond_thumb pattern will correctly > return false itself for a->cond >= 0xe, so it makes no > difference. But maybe it would be more robust for a pattern > like HVC to be self-contained so it doesn't fall through > for cases that really do belong to it but happen to be > required to UNDEF (like IS_USER() == true) ? I agree this should be the rule. E.g. for this IS_USER case, we have successfully decoded HVC and so should not return false. The fact that HVC should raise SIGILL if IS_USER should not be confused with decoding HVC. Other constraints, such as rd != 15 or imod != 0, should continue to return false so that a (potential) grouped insn can match. > OTOH I suppose you could say that when you're writing patterns > like the B_cond_thumb one you know you've underdecoded and must > catch all the theoretical overlaps by writing checks in the trans > function, so as long as you do that correctly you're fine. Yes. r~