Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[BOLT][AArch64] Fix instrumentation deadloop

Authored by yota9 on Fri, Sep 15, 2:08 AM.



According to ARMv8-a architecture reference manual B2.10.5 software
must avoid having any explicit memory accesses between exclusive load
and associated store instruction. Otherwise exclusive monitor might
clear the exclusivity without application-related cause which may
result in the deadloop. Disable instrumentation for such functions,
since between exclusive load and store there might be branches and we
would insert instrumentation snippet which contains loads and stores.

The better solution would be to analyze with BFS finding the exact BBs
between load and store and not instrumenting them. Or even better to
recognize such sequences and replace them with more complex one, e.g.
loading value non exclusively, and for the brach where exclusive store
is made make exclusive load and store sequentially, but for now just
disable instrumentation for such functions completely.

Diff Detail

Event Timeline

yota9 created this revision.Fri, Sep 15, 2:08 AM
yota9 requested review of this revision.Fri, Sep 15, 2:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Sep 15, 2:08 AM

Thank you for this patch!

One very minor comment: in the commit message, you refer to section E2.10.5 of the ArmARM. That section covers Load-Exclusive and Store-Exclusive instructions in AArch32 mode.
This patch targets AArch64. The corresponding section (for AArch64) in the ArmARM seems to be "B2.9.5 Load-Exclusive and Store-Exclusive instruction usage restrictions".


I'm afraid I'm not familiar with the instrumentation feature of BOLT.

Given that typically there are only few instructions (and as you write above, no memory-accessing instructions) in between a load-exclusive and a store-exclusive, I would expect that it should be possible to make most instrumentation work well, even in functions with load-exclusives and store-exclusives?

Maybe only instrumentation that tracks direct conditional branches may be affected?
And even then, maybe it wouldn't be too hard to identify the conditional branches that are between a load-exclusive and a store-exclusive and only stop instrumenting those conditional branches, but still instrument the other conditional branches in the function?

Anyway, to me it feels like that's an improvement that could be done in a follow-on patch?

If all of the above text seems sensible, maybe it would be best to add a "FIXME" comment here to indicate that with a bit more work most instrumentations in functions with load-exclusives/store-exclusives could still be made to work?


It seems the following opcodes are missing?


yota9 marked 2 inline comments as done.Fri, Sep 15, 3:49 AM

Thank you for your review, Kristof! My bad, you're right, at my document it is B2.10.5 (Load-Exclusive and Store-Exclusive instruction usage restrictions), I would fix it in commit message.


I would add FIXME as you suggested.
As for the BOLT we're using multiple load/store instruction, e.g. to preserve the register values that we're using in snippets and for the atomic add for the execution counter. So every snippet would have load or store anyway


Forgot about pair ones, thanks!

yota9 edited the summary of this revision. (Show Details)Fri, Sep 15, 3:49 AM
yota9 updated this revision to Diff 556840.Fri, Sep 15, 3:50 AM
yota9 marked 2 inline comments as done.

Address comments

yota9 updated this revision to Diff 556847.Fri, Sep 15, 4:22 AM

fix typo

yota9 edited the summary of this revision. (Show Details)Fri, Sep 15, 4:22 AM

Thank you, I don't have further comments.
I'm not up to speed enough on BOLT to be able to approve this patch, that will need to be done by one of the other reviewers.

Amir added inline comments.Mon, Sep 18, 10:50 AM

"Exclusive" is ARM-specific term, so it would help to either clarify it a bit here, or come up with a generic term – maybe "isLoadLockStoreConditionalInst"?


Can you please factor out the check into a helper function e.g. hasExclusiveMemop?

yota9 added inline comments.Mon, Sep 18, 10:58 AM

I agree. But we have a bunch of arch-specific interfaces, e.g. is adrp, islea, isRISCVCall. Usually I'm not insisting on such questions, but to my taste "isLoadLockStoreConditionalInst" is a little bit too complicated :)) I like the current variant to be honest, but maybe isAArch64Exclusive is good-enough compromise and in the future add isArch prefix for arch-specific things?

yota9 updated this revision to Diff 557069.Tue, Sep 19, 12:21 PM

Move exlusive check to separate function.

Gentle ping @Amir @rafauler @maksfb

Amir added inline comments.Wed, Sep 20, 10:57 AM

Yes, isAArch64Exclusive is a good compromise.

yota9 updated this revision to Diff 557141.Wed, Sep 20, 12:25 PM

address comments

yota9 updated this revision to Diff 557142.Wed, Sep 20, 12:27 PM


If no further comments, @Amir please approve. Thanks

Amir accepted this revision.Thu, Sep 21, 11:04 AM


This revision is now accepted and ready to land.Thu, Sep 21, 11:04 AM
This revision was automatically updated to reflect the committed changes.