[PATCH] D128387: [hwasan] Add __hwasan_add_frame_record to the hwasan interface

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added a comment.

LGTM



================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:1160
 
+Value *HWAddressSanitizer::getFrameRecordInfo(IRBuilder<> &IRB) {
+  // Prepare ring buffer data.
----------------
you can add this function in a separate patch


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:1186
   if (WithFrameRecord) {
-    StackBaseTag = IRB.CreateAShr(ThreadLong, 3);
-
-    // Prepare ring buffer data.
-    Value *PC = getPC(IRB);
-    Value *SP = getSP(IRB);
-
-    // Mix SP and PC.
-    // Assumptions:
-    // PC is 0x0000PPPPPPPPPPPP  (48 bits are meaningful, others are zero)
-    // SP is 0xsssssssssssSSSS0  (4 lower bits are zero)
-    // We only really need ~20 lower non-zero bits (SSSS), so we mix like this:
-    //       0xSSSSPPPPPPPPPPPP
-    SP = IRB.CreateShl(SP, 44);
-
-    // Store data to ring buffer.
-    Value *RecordPtr =
-        IRB.CreateIntToPtr(ThreadLongMaybeUntagged, IntptrTy->getPointerTo(0));
-    IRB.CreateStore(IRB.CreateOr(PC, SP), RecordPtr);
-
-    // Update the ring buffer. Top byte of ThreadLong defines the size of the
-    // buffer in pages, it must be a power of two, and the start of the buffer
-    // must be aligned by twice that much. Therefore wrap around of the ring
-    // buffer is simply Addr &= ~((ThreadLong >> 56) << 12).
-    // The use of AShr instead of LShr is due to
-    //   https://bugs.llvm.org/show_bug.cgi?id=39030
-    // Runtime library makes sure not to use the highest bit.
-    Value *WrapMask = IRB.CreateXor(
-        IRB.CreateShl(IRB.CreateAShr(ThreadLong, 56), 12, "", true, true),
-        ConstantInt::get(IntptrTy, (uint64_t)-1));
-    Value *ThreadLongNew = IRB.CreateAnd(
-        IRB.CreateAdd(ThreadLong, ConstantInt::get(IntptrTy, 8)), WrapMask);
-    IRB.CreateStore(ThreadLongNew, SlotPtr);
+    if (ClRecordStackHistory == libcall) {
+      // Emit a runtime call into hwasan rather than emitting instructions for
----------------
switch/case


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:1228
 
   if (!ShadowBase) {
+    if (!ThreadLongMaybeUntagged) {
----------------
leonardchan wrote:
> vitalybuka wrote:
> > When I asked about refactoring, you can do all NFC changes in a separate patch,
> > maybe you can reorder ThreadLongMaybeUntagged calculations in NFC patch so you don't have to touch it here
> So this part's a bit awkward bc in the above block, the intermediate variables `SlotPtr` and `ThreadLong` are used for a bunch of other things like `WrapMask` and `StackBaseTag` which aren't needed here. That is, if `ThreadLongMaybeUntagged` isn't set above, then neither would `SlotPtr` or `ThreadLong`. I suppose I could have something like:
> 
> ```
> Value *getThreadLongMaybeUntagged(Value *&SlotPtr, Value *&ThreadLong) {
>     if (!SlotPtr)
>         SlotPtr = getHwasanThreadSlotPtr(IRB, IntptrTy);
>     if (!ThreadLong)
>         ThreadLong = IRB.CreateLoad(IntptrTy, SlotPtr);
>     // Extract the address field from ThreadLong. Unnecessary on AArch64 with TBI.
>     return TargetTriple.isAArch64() ? ThreadLong : untagPointer(IRB, ThreadLong);
> }
> 
>   Value *ThreadLongMaybeUntagged = nullptr, *SlotPtr = nullptr, *ThreadLong = nullptr;
>   if (WithFrameRecord) {
>     if (ClRecordStackHistoryWithCalls) {
>         ....
>     } else {
>         ThreadLongMaybeUntagged = getThreadLongMaybeUntagged(SlotPtr, ThreadLong);
>         ....
>     }
>   }
> 
>   if (!ShadowBase) {
>     if (!ThreadLongMaybeUntagged) {
>       ThreadLongMaybeUntagged = getThreadLongMaybeUntagged(SlotPtr, ThreadLong);
>       ...
> ```
> 
> where I can save intermediate values?
function local lambda works for me
```
auto getThreadLongMaybeUntagged = [&]() {
    if (!SlotPtr)
        SlotPtr = getHwasanThreadSlotPtr(IRB, IntptrTy);
    if (!ThreadLong)
        ThreadLong = IRB.CreateLoad(IntptrTy, SlotPtr);
    // Extract the address field from ThreadLong. Unnecessary on AArch64 with TBI.
    return TargetTriple.isAArch64() ? ThreadLong : untagPointer(IRB, ThreadLong);
}
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128387/new/

https://reviews.llvm.org/D128387