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

[Leonard Chan via Phabricator via llvm-commits]

leonardchan added a comment.

Addressed some comments. Also updated the current stack history codegen path such that `getFrameRecordInfo` can be reused. Although this changes some test cases. If the patch is too big with this, I think I can move that into a separate one.



================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:183-187
 static cl::opt<bool>
     ClRecordStackHistory("hwasan-record-stack-history",
                          cl::desc("Record stack frames with tagged allocations "
                                   "in a thread-local ring buffer"),
                          cl::Hidden, cl::init(true));
----------------
vitalybuka wrote:
> maybe better to convert hwasan-record-stack-history into enum?
> hwasan-record-stack-history-with-calls is meaningless if hwasan-record-stack-history=0
Done and updated tests. I couldn't think of a better enum value for the default behavior other than `instr` :/


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:1210
+                                            IntptrTy->getPointerTo(0));
+      IRB.CreateStore(IRB.CreateOr(PC, SP), RecordPtr);
+
----------------
vitalybuka wrote:
> mcgrathr wrote:
> > It seems better to call the subroutine you just broke out above rather than to have its logic still open-coded here as well.
> > Conversely, perhaps just move this code up before the conditional and reuse the same `FrameRecordInfo` local variable in both the libcall and direct-store cases, rather than having a subroutine.
> > 
> Also looks like can be inserted twice
> Value *ThreadLong = IRB.CreateLoad(IntptrTy, SlotPtr);
> It seems better to call the subroutine you just broke out above rather than to have its logic still open-coded here as well. Conversely, perhaps just move this code up before the conditional and reuse the same FrameRecordInfo local variable in both the libcall and direct-store cases, rather than having a subroutine.

Can do. I initially wanted to leave that out since it would mean this patch changes the IR in the prologue slightly and updating some tests that I could save for another patch. Currently, the order is (1) `inttoptr` (2) `or PC, SP` (3) `store`. But reusing the subroutine or `FrameRecordInfo` would change the order to (1) `or SP, PC` (2) `inttoptr` (3) `store`, but I think that shouldn't affect lowering from IR much.

> Also looks like can be inserted twice
> Value *ThreadLong = IRB.CreateLoad(IntptrTy, SlotPtr);

I think the second `Value *ThreadLong = IRB.CreateLoad(IntptrTy, SlotPtr);` in the conditional below shouldn't happen if `ThreadLongMaybeUntagged` is set, which will be set in this block where the first `CreateLoad` would happen since it looks like `ThreadLongMaybeUntagged = TargetTriple.isAArch64() ? ThreadLong : untagPointer(IRB, ThreadLong)` should never be nullptr, unless I'm perhaps missing something.


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:1228
 
   if (!ShadowBase) {
+    if (!ThreadLongMaybeUntagged) {
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128387