This is an archive of the discontinued LLVM Phabricator instance.

[BOLT][AArch64] Fix instrumentation deadloop
ClosedPublic

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

Details

Summary

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.Sep 15 2023, 2:08 AM
yota9 requested review of this revision.Sep 15 2023, 2:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 15 2023, 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".

bolt/lib/Passes/Instrumentation.cpp
313

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?

bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
292

It seems the following opcodes are missing?

LDAXPW, LDAXPX, LDXPW, LDXPX,
STLXPW, STLXPX, STXPW, STXPX.

yota9 marked 2 inline comments as done.Sep 15 2023, 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.

bolt/lib/Passes/Instrumentation.cpp
313

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

bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
292

Forgot about pair ones, thanks!

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

Address comments

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

fix typo

yota9 edited the summary of this revision. (Show Details)Sep 15 2023, 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.Sep 18 2023, 10:50 AM
bolt/include/bolt/Core/MCPlusBuilder.h
629

"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"?

bolt/lib/Passes/Instrumentation.cpp
315

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

yota9 added inline comments.Sep 18 2023, 10:58 AM
bolt/include/bolt/Core/MCPlusBuilder.h
629

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.Sep 19 2023, 12:21 PM

Move exlusive check to separate function.

Gentle ping @Amir @rafauler @maksfb

Amir added inline comments.Sep 20 2023, 10:57 AM
bolt/include/bolt/Core/MCPlusBuilder.h
629

Yes, isAArch64Exclusive is a good compromise.

bolt/lib/Passes/Instrumentation.cpp
310–311
yota9 updated this revision to Diff 557141.Sep 20 2023, 12:25 PM

address comments

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

clang-format

If no further comments, @Amir please approve. Thanks

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

Thanks!

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