[PATCH] D116511: [clang-cl] Support the /HOTPATCH flag

Wed Jan 12 09:10:06 PST 2022

aganea marked 2 inline comments as done.
aganea added inline comments.

================
Comment at: clang/test/CodeGenCXX/debug-info-hotpatch-arm.cpp:13
+// RUN: llvm-pdbutil dump -symbols %t.obj | FileCheck %s --check-prefix=HOTPATCH
+// ERR-HOTPATCH: error: unsupported option '/hotpatch' for target
+// HOTPATCH: S_COMPILE3 [size = [[#]]]
----------------
hans wrote:
> Does MSVC error for ARM/ARM64 too, or does it just ignore the flag?
It prints a warning:
```
D:\git\llvm-project>cl /c main.cpp /hotpatch
Microsoft (R) C/C++ Optimizing Compiler Version 19.29.30138 for ARM64
Copyright (C) Microsoft Corporation.  All rights reserved.

cl : Command line warning D9002 : ignoring unknown option '/hotpatch'
main.cpp
```
But in our case, the issue is that `PATCHABLE_OP` isn't supported on ARM backend, so it ends up asserting in `D:/git/llvm-project/release/lib/Target/AArch64/AArch64GenMCCodeEmitter.inc` in `AArch64MCCodeEmitter::getBinaryCodeForInstr`. There's perhaps a (better?) way for shortcutting the use of `/hotpatch` on ARM, instead of erroring-out like today.

Should we be more clear in the message, saying that hotpatching is supported but we don't the flag? Or just consume and ignore it?

================
Comment at: clang/test/CodeGenCXX/debug-info-hotpatch-arm.cpp:19
+///
+// RUN: %clang_cl --target=aarch64-pc-windows-msvc /c %s -o %t.obj
+// RUN: llvm-pdbutil dump -symbols %t.obj | FileCheck %s --check-prefix=NO-HOTPATCH
----------------
hans wrote:
> nit: We still need -- before %s with clang-cl (%s could be /Users/... and get interpreted as a flag), and so it needs to come last. Same for the line in debug-info-hotpatch.cpp.
Thanks will do!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116511