This is an archive of the discontinued LLVM Phabricator instance.

[Attr] Add support for the `ms_hook_prologue` attribute.
ClosedPublic

Authored by cdavis5x on May 4 2016, 1:55 AM.

Details

Summary

Based on a patch by Michael Mueller.

This attribute specifies that a function can be hooked or patched. This
mechanism was originally devised by Microsoft for hotpatching their
binaries (which they're constantly updating to stay ahead of crackers,
script kiddies, and other ne'er-do-wells on the Internet), but it's now
commonly abused by Windows programs that want to hook API functions. It
is for this reason that this attribute was added to GCC--hence the name,
ms_hook_prologue.

Depends on D19908.

Diff Detail

Repository
rL LLVM

Event Timeline

cdavis5x updated this revision to Diff 56108.May 4 2016, 1:55 AM
cdavis5x retitled this revision from to [Attr] Add support for the `ms_hook_prologue` attribute..
cdavis5x updated this object.
cdavis5x added reviewers: aaron.ballman, rnk.
cdavis5x added a subscriber: cfe-commits.
aaron.ballman edited edge metadata.May 4 2016, 12:06 PM

Generally, I think this should make use of the patchable function functionality from D19046 when lowering, but I do have some comments on the attribute itself as well.

include/clang/Basic/Attr.td
2032 ↗(On Diff #56108)

Does this attribute appertain to all targets, or only targets that MSVC supports?

2035 ↗(On Diff #56108)

Please, no undocumented new attributes.

cdavis5x updated this revision to Diff 57389.May 16 2016, 1:23 PM
cdavis5x edited edge metadata.
  • Use Sanjoy's patchable-function attribute.
  • Add documentation for this new attribute.
cdavis5x marked an inline comment as done.May 16 2016, 1:26 PM
cdavis5x added inline comments.
include/clang/Basic/Attr.td
2032 ↗(On Diff #56108)

Only to CPUs that MSVC supports, but it works on all OSes--even non-Windows ones.

I should mention that this attribute was specifically added to GCC so Wine could use it.

The patch is missing semantic test cases. I would like to see tests of the attribute being accepted, as well as rejected because the subject was incorrect, args were provided, etc.

Also, I believe there are attributes which this attribute should not be combined with, such as __declspec(naked). I think we should diagnose in that case (MSVC does not because the entire object file is hotpatched, not individual functions). e.g., __declspec(naked) void f(void) __attribute__((ms_hook_prologue));

include/clang/Basic/Attr.td
2032 ↗(On Diff #57389)

I am wondering whether we want this to be a target-specific attribute or not. Right now, this attribute will be silently accepted for CPUs that MSVC does not support, for instance. If we made the attribute target-specific, then users would get appropriate diagnostics for those architectures.

cdavis5x updated this revision to Diff 57853.May 19 2016, 1:35 PM
cdavis5x marked an inline comment as done.
  • Add Sema tests for the ms_hook_prologue attribute.
  • Disallow ms_hook_prologue on architectures that Windows doesn't support.
  • Disallow ms_hook_prologue with the naked and always_inline attributes.

For now, I've disallowed it with naked and always_inline/__forceinline attributes. Do any other attributes affect prologue generation in a way that might interfere with ms_hook_prologue? AFAICT, GCC only disallows ms_hook_prologue on a) nested functions and b) functions compiled with -mfentry (neither of which we support, AFAIK).

include/clang/Basic/Attr.td
2032 ↗(On Diff #57389)

For now, I've limited it to architectures that Windows supports.

aaron.ballman accepted this revision.May 26 2016, 4:55 AM
aaron.ballman edited edge metadata.

LGTM (when Sanjoy's patch goes in), with two minor nits.

Can you also add a test that the attribute fails on one of the target architectures Windows does not support?

include/clang/Basic/AttrDocs.td
506 ↗(On Diff #57853)

Should document that we disallow it in conjunction with naked, always_inline, and __forceinline.

This revision is now accepted and ready to land.May 26 2016, 4:55 AM
cdavis5x updated this revision to Diff 66729.Aug 3 2016, 4:08 PM
cdavis5x edited edge metadata.

Update for merge conflicts.

  • Add a blurb to the docs advising against using this attribute with __forceinline, always_inline, or naked.
cdavis5x marked an inline comment as done.Aug 3 2016, 4:11 PM
cdavis5x added inline comments.
include/clang/Basic/AttrDocs.td
560 ↗(On Diff #66729)

Done.

This revision was automatically updated to reflect the committed changes.