Page MenuHomePhabricator

[clang-cl] Support the /HOTPATCH flag
ClosedPublic

Authored by aganea on Jan 2 2022, 5:52 PM.

Details

Summary

This patch adds support for the /HOTPATCH flag: https://docs.microsoft.com/sv-se/cpp/build/reference/hotpatch-create-hotpatchable-image?view=msvc-170&viewFallbackFrom=vs-2019

The flag is translated to a new -fms-hotpatch flag, which in turn adds a patchable-function attribute for each function in the TU. This is then picked up by the PatchableFunction pass which would generate a TargetOpcode::PATCHABLE_OP of minsize = 2 (which means the target instruction must resolve to at least two bytes). TargetOpcode::PATCHABLE_OP is only implemented for x86/x64. When targetting ARM/ARM64, /hotpatch isn't required (instructions are always 2/4 bytes and suitable for hotpatching).

Additionally, when using /Z7, we generate a 'hot patchable' flag in the CodeView debug stream, in the S_COMPILE3 record. This flag is then picked up by LLD (or link.exe) and is used in conjunction with the linker /FUNCTIONPADMIN flag to generate extraneous space before each function, to accommodate for live patching. Please see: https://github.com/llvm/llvm-project/blob/d703b922961e0d02a5effdd4bfbb23ad50a3cc9f/lld/COFF/Writer.cpp#L1298

The end result is that one can finally use Live++ or Recode along with clang-cl.

NOTE: It seems that MSVC cl.exe always enables /HOTPATCH on x64 by default, although if we did the same I thought we might generate sub-optimal code (if this flag was active by default). Additionally, MSVC always generates a .debug$S section and a S_COMPILE3 record. Therefore, the MSVC command-line cl /c file.cpp would have to be written with Clang such as clang-cl /c file.cpp /HOTPATCH /Z7 in order to obtain the same result.

Diff Detail

Event Timeline

aganea created this revision.Jan 2 2022, 5:52 PM
aganea requested review of this revision.Jan 2 2022, 5:52 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 2 2022, 5:52 PM
aganea edited the summary of this revision. (Show Details)Jan 2 2022, 5:59 PM
aganea updated this revision to Diff 397063.Jan 3 2022, 7:24 AM
aganea added a subscriber: saudi.

Fix tests.

hans added a comment.Jan 7 2022, 3:49 AM

Cool!

I'd suggesting adding a note to llvm/docs/ReleaseNotes.rst at the same time :-)

clang/include/clang/Basic/CodeGenOptions.def
142

nit: I'd s/would generate/generages/

clang/include/clang/Driver/Options.td
2501

Is this flag also exposed as a driver flag, or is it cc1 only?

I wonder if we should go for a less generic name here, perhaps -fms-hotpatch? For example, the flag above is also about hotpatching, but a different mechanism.

clang/lib/CodeGen/BackendUtil.cpp
648

Since it's currently only supported on x86_64, we should probably diagnose trying to use it for other target somewhere.

clang/lib/Driver/ToolChains/Clang.cpp
6003–6005

Can we also pass /functionpadmin when clang-cl invokes the linker, like cl does?

clang/test/Driver/cl-options.c
488–489

We should drop this.

aganea updated this revision to Diff 399168.Jan 11 2022, 7:03 PM
aganea marked 4 inline comments as done.
aganea edited the summary of this revision. (Show Details)

As suggested by @hans

Also added test coverage for ARM/ARM64.

aganea added inline comments.Jan 11 2022, 7:04 PM
clang/include/clang/Driver/Options.td
2501

Added CoreOption. Changed to -fms-hotpatch.

clang/lib/CodeGen/BackendUtil.cpp
648

Actually the whole thing works on ARM/ARM64, it just that these targets don't need the PATCHABLE_OP thing (since they always generate 16-bit/32-bit instructions).

hans accepted this revision.Jan 12 2022, 8:22 AM

lgtm with comments

clang/test/CodeGenCXX/debug-info-hotpatch-arm.cpp
14

Does MSVC error for ARM/ARM64 too, or does it just ignore the flag?

20

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.

This revision is now accepted and ready to land.Jan 12 2022, 8:22 AM
aganea marked 2 inline comments as done.Jan 12 2022, 9:10 AM
aganea added inline comments.
clang/test/CodeGenCXX/debug-info-hotpatch-arm.cpp
14

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?

20

Thanks will do!

hans added inline comments.Jan 13 2022, 9:03 AM
clang/test/CodeGenCXX/debug-info-hotpatch-arm.cpp
14

I think the most user-friendly thing would be to consume and ignore it.

This revision was landed with ongoing or failed builds.Jan 20 2022, 9:57 AM
This revision was automatically updated to reflect the committed changes.
aganea marked 3 inline comments as done.