This is an archive of the discontinued LLVM Phabricator instance.

[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

Unit TestsFailed

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
2493

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
5998

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

clang/test/Driver/cl-options.c
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
2493

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
13 ↗(On Diff #399168)

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

19 ↗(On Diff #399168)

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
13 ↗(On Diff #399168)

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?

19 ↗(On Diff #399168)

Thanks will do!

hans added inline comments.Jan 13 2022, 9:03 AM
clang/test/CodeGenCXX/debug-info-hotpatch-arm.cpp
13 ↗(On Diff #399168)

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.