[herd,aarch64] Adding ctrl to indirect branches

[artkhyzha]

This is a draft PR aimed at making indirect branches create control dependencies.

The idea is to simply let the indirect branches to create a "commit" type of an event; then the aarch64deps.cat should automatically draw ctrl edges.

A possible variation of the approach could be to create a separate kind of events for indirect branches and to alter aarch64deps.cat accordingly -- simply to keep the old and the new events separated.

[jalglave]

thanks @artkhyzha this looks good to me.
@maranget please could you regress this?
thanks in advance,
Jade

[artkhyzha]

Seeing @jalglave's endorsement, I am marking this non-draft.

What do you think about this change, @maranget? Is there anything I could help with review-wise?

[maranget]

In my opinion commit_bcc ii >>= fun () -> has to be added in front of line 2008 below.

[artkhyzha]

Thanks you -- makes sense to add it there.

(This makes me wonder if there is any unit test to compose for the new functionality.)

[maranget]

It may a good idea to add a test whose outcome changes with the new control dependency:

AArch64 MP+BR
{
ins_t *y;
0:X1=x; 0:X3=y; 0:X2=P1:L0;
1:X0=y; 1:X2=x;
}
 P0           | P1          ;
 MOV W0,#1    | ADR X4,L1   ;
 STR W0,[X1]  | STR X4,[X0] ;
 STLR X2,[X3] | DMB LD      ;
              | LDR X1,[X0] ;
              | BR  X1      ;
              |L0:          ; 
              | ISB         ;
              | LDR W3,[X2] ;
              |L1:          ;
              | CMP X1,X4   ;
              | CSET W5,NE  ;
exists (1:X5=1 /\ 1:X3=0)

If I am not wrong, the test was allowed and is now forbidden.

For the record, I discovered the problem fixed by PR #642 on a similar test.

[artkhyzha]

Thank you for this, @maranget, it's a great suggestion. I'd include these three test. The first two are just minor tweaks of your suggestion, I'll explain in a bit why I tweaked them. Where do you think these should go? Should they be in catalogue/aarch64/... or herd/tests/AArch64/...?

AArch64 MP+rel+br-ctrlisb-1
{
ins_t *y;
0:X2=x; 0:X4=y; 0:X3=P1:L0;
1:X2=x; 1:X4=y;
}
P0           | P1          ;
MOV W1,#1    | ADR X5,L1   ;
STR W1,[X2]  | STR X5,[X4] ;
STLR X3,[X4] | LDR X3,[X4] ;
             | BR  X3      ;
             |L0:          ; 
             | ISB         ;
             | MOV W0,#1   ;
             | LDR W1,[X2] ;
             |L1:          ;
exists (1:X0=1 /\ 1:X1=0)

AArch64 MP+rel+br-ctrlisb-2
{
ins_t *y;
0:X2=x; 0:X4=y; 0:X3=P1:L0;
1:X2=x; 1:X4=y;
}
P0           | P1          ;
MOV W1,#1    | ADR X5,L1   ;
STR W1,[X2]  | STR X5,[X4] ;
STLR X3,[X4] | LDR X3,[X4] ;
             | BR  X3      ;
             |L0:          ;
             | ISB         ;
             | MOV W0,#1   ;
             |L1:          ;
             | LDR W1,[X2] ;
exists (1:X0=1 /\ 1:X1=0)

AArch64 LB+rel+br-ctrl
{
ins_t *y;
0:X2=x; 0:X4=y; 0:X3=P1:L0;
1:X2=x; 1:X4=y;
}
P0           |P1           ;
LDR W1,[X2]  | ADR X5,L1   ;
STLR X3,[X4] | STR X5,[X4] ;
             | LDR X3,[X4] ;
             | BR X3       ;
             |L0:          ;
             | MOV W0,#1   ;
             | STR W0,[X2] ;
             |L1:          ;
exists (0:X1=1 /\ 1:X0=1)

[artkhyzha]

So I decided to go ahead and keep the aforementioned tests as unit tests. (Will gladly change to another approach, if it's preferred.)

@maranget, would you say there are more things to do for this PR?

[maranget]

Hi @artkhyzha, the new tests are in the right place. I'll perform a full regression before merging, as asked by @jalglave. I am lacking time at them moment. Please bear with me :)

[maranget]

Complete regression went fine yesterday night. There is one change, which is expected since the test contains one indirect jump. Rebased and merged. Thanks @artkhyzha.

[artkhyzha]

Thank you, @maranget!