Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[clang-cl] Support the /HOTPATCH flag

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



This patch adds support for the /HOTPATCH flag:

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:

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


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


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


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.


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


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


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

Added CoreOption. Changed to -fms-hotpatch.


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


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


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.

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'

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/ 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?


Thanks will do!

hans added inline comments.Jan 13 2022, 9:03 AM

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.