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

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added a comment.

In D128387#3623787 <https://reviews.llvm.org/D128387#3623787>, @leonardchan wrote:

> In D128387#3603205 <https://reviews.llvm.org/D128387#3603205>, @vitalybuka wrote:
>
>> Could you please move refactoring (like extraction emitPrologue) into a separate CL?
>>
>> We also need one test for runtime and one test for generated IR.
>
> Should I break this up into 2 compiler-rt and llvm patches?

I don't have preference regarding compiler-rt / llvm split. Seems having them together is useful.
It's more about moving as much as possible into NFC patches. Also in this snapshot it's not obvious what can be move out of the patch.



================
Comment at: compiler-rt/lib/hwasan/hwasan.cpp:579
 
+void __hwasan_record_frame_record(u64 frame_record_info) {
+  Thread *t = GetCurrentThread();
----------------
can have a nicer name?
maybe __hwasan_add_frame_record


================
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));
----------------
maybe better to convert hwasan-record-stack-history into enum?
hwasan-record-stack-history-with-calls is meaningless if hwasan-record-stack-history=0


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:1210
+                                            IntptrTy->getPointerTo(0));
+      IRB.CreateStore(IRB.CreateOr(PC, SP), RecordPtr);
+
----------------
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);


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128387