Re: [PATCH] D19909: [Attr] Add support for the `ms_hook_prologue` attribute.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL278050: [Attr] Add support for the `ms_hook_prologue`
attribute. (authored by cdavis).
Changed prior to commit:
https://reviews.llvm.org/D19909?vs=66729=67224#toc
Repository:
rL LLVM
https://reviews.llvm.org/D19909
Files:
cfe/trunk/include/clang/Basic/Attr.td
cfe/trunk/include/clang/Basic/AttrDocs.td
cfe/trunk/lib/CodeGen/TargetInfo.cpp
cfe/trunk/lib/Sema/SemaDeclAttr.cpp
cfe/trunk/test/CodeGen/function-attributes.c
cfe/trunk/test/Sema/attr-ms-hook-prologue-wrong-arch.c
cfe/trunk/test/Sema/attr-ms-hook-prologue.c
Index: cfe/trunk/lib/CodeGen/TargetInfo.cpp
===
--- cfe/trunk/lib/CodeGen/TargetInfo.cpp
+++ cfe/trunk/lib/CodeGen/TargetInfo.cpp
@@ -1779,6 +1779,10 @@
llvm::Function *Fn = cast(GV);
Fn->setCallingConv(llvm::CallingConv::X86_INTR);
}
+if (FD->hasAttr()) {
+ llvm::Function *Fn = cast(GV);
+ Fn->addFnAttr("patchable-function", "ms-hotpatch");
+}
}
}
@@ -2109,6 +2113,10 @@
llvm::Function *Fn = cast(GV);
Fn->setCallingConv(llvm::CallingConv::X86_INTR);
}
+ if (FD->hasAttr()) {
+llvm::Function *Fn = cast(GV);
+Fn->addFnAttr("patchable-function", "ms-hotpatch");
+ }
}
}
};
Index: cfe/trunk/lib/Sema/SemaDeclAttr.cpp
===
--- cfe/trunk/lib/Sema/SemaDeclAttr.cpp
+++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp
@@ -1664,15 +1664,6 @@
D->addAttr(CA);
}
-static void handleNakedAttr(Sema , Decl *D, const AttributeList ) {
- if (checkAttrMutualExclusion(S, D, Attr.getRange(),
- Attr.getName()))
-return;
-
- D->addAttr(::new (S.Context) NakedAttr(Attr.getRange(), S.Context,
- Attr.getAttributeSpellingListIndex()));
-}
-
static void handleNoReturnAttr(Sema , Decl *D, const AttributeList ) {
if (hasDeclarator(D)) return;
@@ -3673,7 +3664,9 @@
static void handleAlwaysInlineAttr(Sema , Decl *D,
const AttributeList ) {
if (checkAttrMutualExclusion(S, D, Attr.getRange(),
- Attr.getName()))
+ Attr.getName()) ||
+ checkAttrMutualExclusion(S, D, Attr.getRange(),
+ Attr.getName()))
return;
if (AlwaysInlineAttr *Inline = S.mergeAlwaysInlineAttr(
@@ -5552,7 +5545,8 @@
handleHotAttr(S, D, Attr);
break;
case AttributeList::AT_Naked:
-handleNakedAttr(S, D, Attr);
+handleSimpleAttributeWithExclusions(S, D, Attr);
break;
case AttributeList::AT_NoReturn:
handleNoReturnAttr(S, D, Attr);
@@ -5780,6 +5774,9 @@
break;
case AttributeList::AT_LayoutVersion:
handleLayoutVersion(S, D, Attr);
+ case AttributeList::AT_MSHookPrologue:
+handleSimpleAttributeWithExclusions(S, D, Attr);
break;
case AttributeList::AT_MSNoVTable:
handleSimpleAttribute(S, D, Attr);
Index: cfe/trunk/include/clang/Basic/AttrDocs.td
===
--- cfe/trunk/include/clang/Basic/AttrDocs.td
+++ cfe/trunk/include/clang/Basic/AttrDocs.td
@@ -548,6 +548,22 @@
}];
}
+def MSHookPrologueDocs : Documentation {
+ let Category = DocCatFunction;
+ let Content = [{
+The ``ms_hook_prologue`` attribute marks a function as "hotpatchable" according
+to conventions used on Windows. Specifically, enough space will be ensured
+in the prologue for a short jump, and an architecturally dependently sized
+patch space will be reserved prior to the entry point. See the documentation
+for the `/HOTPATCH`_ switch on MSDN.
+
+This attribute cannot be used in conjunction with the ``naked``,
+``always_inline``, or ``__forceinline`` attributes.
+
+.. _`/HOTPATCH`: https://msdn.microsoft.com/en-us/library/ms173507.aspx
+ }];
+}
+
def NoDebugDocs : Documentation {
let Category = DocCatVariable;
let Content = [{
Index: cfe/trunk/include/clang/Basic/Attr.td
===
--- cfe/trunk/include/clang/Basic/Attr.td
+++ cfe/trunk/include/clang/Basic/Attr.td
@@ -257,6 +257,7 @@
def TargetMSP430 : TargetArch<["msp430"]>;
def TargetX86 : TargetArch<["x86"]>;
def TargetAnyX86 : TargetArch<["x86", "x86_64"]>;
+def TargetWindowsArches : TargetArch<["x86", "x86_64", "arm", "thumb"]>;
def TargetWindows : TargetArch<["x86", "x86_64", "arm", "thumb"]> {
let OSes = ["Win32"];
}
@@ -2069,6 +2070,12 @@
// Microsoft-related attributes
+def MSHookPrologue : InheritableAttr, TargetSpecificAttr {
+ let Spellings =
Re: [PATCH] D19909: [Attr] Add support for the `ms_hook_prologue` attribute.
cdavis5x marked an inline comment as done. Comment at: include/clang/Basic/AttrDocs.td:560 @@ +559,3 @@ + +This attribute cannot be used in conjunction with the ``naked``, +``always_inline``, or ``__forceinline`` attributes. Done. https://reviews.llvm.org/D19909 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19909: [Attr] Add support for the `ms_hook_prologue` attribute.
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. 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? Comment at: include/clang/Basic/AttrDocs.td:506 @@ +505,3 @@ + +.. _`/HOTPATCH`: https://msdn.microsoft.com/en-us/library/ms173507.aspx + }]; Should document that we disallow it in conjunction with naked, always_inline, and __forceinline. http://reviews.llvm.org/D19909 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19909: [Attr] Add support for the `ms_hook_prologue` attribute.
cdavis5x updated this revision to Diff 57853.
cdavis5x marked an inline comment as done.
cdavis5x added a comment.
- 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.
http://reviews.llvm.org/D19909
Files:
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
lib/CodeGen/TargetInfo.cpp
lib/Sema/SemaDeclAttr.cpp
test/CodeGen/function-attributes.c
test/Sema/attr-ms-hook-prologue.c
Index: test/Sema/attr-ms-hook-prologue.c
===
--- /dev/null
+++ test/Sema/attr-ms-hook-prologue.c
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -triple i386-pc-linux -fms-extensions -fsyntax-only -verify %s
+
+int __attribute__((ms_hook_prologue)) foo(int a, int b) {
+ return a+b;
+}
+
+// expected-note@+2{{conflicting attribute is here}}
+// expected-error@+1{{'naked' and 'ms_hook_prologue' attributes are not compatible}}
+__declspec(naked) int __attribute__((ms_hook_prologue)) bar(int a, int b) {
+}
+
+// expected-note@+2{{conflicting attribute is here}}
+// expected-error@+1{{'__forceinline' and 'ms_hook_prologue' attributes are not compatible}}
+__forceinline int __attribute__((ms_hook_prologue)) baz(int a, int b) {
+ return a-b;
+}
+
+// expected-warning@+1{{'ms_hook_prologue' attribute only applies to functions}}
+int x __attribute__((ms_hook_prologue));
+
+// expected-error@+1{{'ms_hook_prologue' attribute takes no arguments}}
+int f(int a, int b) __attribute__((ms_hook_prologue(2)));
Index: test/CodeGen/function-attributes.c
===
--- test/CodeGen/function-attributes.c
+++ test/CodeGen/function-attributes.c
@@ -108,11 +108,18 @@
_setjmp(0);
}
+// CHECK-LABEL: define void @f21
+// CHECK: [[HOTPATCH:#[0-9]+]]
+// CHECK: {
+void __attribute__((ms_hook_prologue)) f21(void) {
+}
+
// CHECK: attributes [[NUW]] = { nounwind optsize{{.*}} }
// CHECK: attributes [[AI]] = { alwaysinline nounwind optsize{{.*}} }
// CHECK: attributes [[NUW_OS_RN]] = { nounwind optsize readnone{{.*}} }
// CHECK: attributes [[ALIGN]] = { nounwind optsize alignstack=16{{.*}} }
// CHECK: attributes [[RT]] = { nounwind optsize returns_twice{{.*}} }
+// CHECK: attributes [[HOTPATCH]] = { nounwind optsize{{.*}}"patchable-function"="ms-hotpatch"{{.*}} }
// CHECK: attributes [[NR]] = { noreturn optsize }
// CHECK: attributes [[NUW_RN]] = { nounwind optsize readnone }
// CHECK: attributes [[RT_CALL]] = { optsize returns_twice }
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -1663,15 +1663,6 @@
D->addAttr(CA);
}
-static void handleNakedAttr(Sema , Decl *D, const AttributeList ) {
- if (checkAttrMutualExclusion(S, D, Attr.getRange(),
- Attr.getName()))
-return;
-
- D->addAttr(::new (S.Context) NakedAttr(Attr.getRange(), S.Context,
- Attr.getAttributeSpellingListIndex()));
-}
-
static void handleNoReturnAttr(Sema , Decl *D, const AttributeList ) {
if (hasDeclarator(D)) return;
@@ -3672,7 +3663,9 @@
static void handleAlwaysInlineAttr(Sema , Decl *D,
const AttributeList ) {
if (checkAttrMutualExclusion(S, D, Attr.getRange(),
- Attr.getName()))
+ Attr.getName()) ||
+ checkAttrMutualExclusion(S, D, Attr.getRange(),
+ Attr.getName()))
return;
if (AlwaysInlineAttr *Inline = S.mergeAlwaysInlineAttr(
@@ -5525,7 +5518,8 @@
handleHotAttr(S, D, Attr);
break;
case AttributeList::AT_Naked:
-handleNakedAttr(S, D, Attr);
+handleSimpleAttributeWithExclusions(S, D, Attr);
break;
case AttributeList::AT_NoReturn:
handleNoReturnAttr(S, D, Attr);
@@ -5748,6 +5742,10 @@
break;
// Microsoft attributes:
+ case AttributeList::AT_MSHookPrologue:
+handleSimpleAttributeWithExclusions(S, D, Attr);
+break;
case AttributeList::AT_MSNoVTable:
handleSimpleAttribute(S, D, Attr);
break;
Index: lib/CodeGen/TargetInfo.cpp
===
--- lib/CodeGen/TargetInfo.cpp
+++ lib/CodeGen/TargetInfo.cpp
@@ -1749,6 +1749,10 @@
llvm::Function *Fn = cast(GV);
Fn->setCallingConv(llvm::CallingConv::X86_INTR);
}
+if (FD->hasAttr()) {
+ llvm::Function *Fn = cast(GV);
+ Fn->addFnAttr("patchable-function", "ms-hotpatch");
+}
}
}
@@ -2079,6 +2083,10 @@
llvm::Function *Fn
Re: [PATCH] D19909: [Attr] Add support for the `ms_hook_prologue` attribute.
cdavis5x added a comment.
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).
Comment at: include/clang/Basic/Attr.td:2032
@@ -2031,1 +2031,3 @@
+def MSHookPrologue : InheritableAttr {
+ let Spellings = [GCC<"ms_hook_prologue">];
aaron.ballman wrote:
> 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.
For now, I've limited it to architectures that Windows supports.
http://reviews.llvm.org/D19909
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19909: [Attr] Add support for the `ms_hook_prologue` attribute.
aaron.ballman added a comment.
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));`
Comment at: include/clang/Basic/Attr.td:2032
@@ -2031,1 +2031,3 @@
+def MSHookPrologue : InheritableAttr {
+ let Spellings = [GCC<"ms_hook_prologue">];
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.
http://reviews.llvm.org/D19909
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19909: [Attr] Add support for the `ms_hook_prologue` attribute.
cdavis5x marked an inline comment as done.
Comment at: include/clang/Basic/Attr.td:2032
@@ -2031,1 +2031,3 @@
+def MSHookPrologue : InheritableAttr {
+ let Spellings = [GCC<"ms_hook_prologue">];
aaron.ballman wrote:
> Does this attribute appertain to all targets, or only targets that MSVC
> supports?
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.
http://reviews.llvm.org/D19909
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19909: [Attr] Add support for the `ms_hook_prologue` attribute.
cdavis5x updated this revision to Diff 57389.
cdavis5x added a comment.
- Use Sanjoy's `patchable-function` attribute.
- Add documentation for this new attribute.
http://reviews.llvm.org/D19909
Files:
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
lib/CodeGen/TargetInfo.cpp
lib/Sema/SemaDeclAttr.cpp
test/CodeGen/function-attributes.c
Index: test/CodeGen/function-attributes.c
===
--- test/CodeGen/function-attributes.c
+++ test/CodeGen/function-attributes.c
@@ -108,11 +108,18 @@
_setjmp(0);
}
+// CHECK-LABEL: define void @f21
+// CHECK: [[HOTPATCH:#[0-9]+]]
+// CHECK: {
+void __attribute__((ms_hook_prologue)) f21(void) {
+}
+
// CHECK: attributes [[NUW]] = { nounwind optsize{{.*}} }
// CHECK: attributes [[AI]] = { alwaysinline nounwind optsize{{.*}} }
// CHECK: attributes [[NUW_OS_RN]] = { nounwind optsize readnone{{.*}} }
// CHECK: attributes [[ALIGN]] = { nounwind optsize alignstack=16{{.*}} }
// CHECK: attributes [[RT]] = { nounwind optsize returns_twice{{.*}} }
+// CHECK: attributes [[HOTPATCH]] = { nounwind optsize{{.*}}"patchable-function"="ms-hotpatch"{{.*}} }
// CHECK: attributes [[NR]] = { noreturn optsize }
// CHECK: attributes [[NUW_RN]] = { nounwind optsize readnone }
// CHECK: attributes [[RT_CALL]] = { optsize returns_twice }
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -5748,6 +5748,9 @@
break;
// Microsoft attributes:
+ case AttributeList::AT_MSHookPrologue:
+handleSimpleAttribute(S, D, Attr);
+break;
case AttributeList::AT_MSNoVTable:
handleSimpleAttribute(S, D, Attr);
break;
Index: lib/CodeGen/TargetInfo.cpp
===
--- lib/CodeGen/TargetInfo.cpp
+++ lib/CodeGen/TargetInfo.cpp
@@ -1749,6 +1749,10 @@
llvm::Function *Fn = cast(GV);
Fn->setCallingConv(llvm::CallingConv::X86_INTR);
}
+if (FD->hasAttr()) {
+ llvm::Function *Fn = cast(GV);
+ Fn->addFnAttr("patchable-function", "ms-hotpatch");
+}
}
}
@@ -2079,6 +2083,10 @@
llvm::Function *Fn = cast(GV);
Fn->setCallingConv(llvm::CallingConv::X86_INTR);
}
+ if (FD->hasAttr()) {
+llvm::Function *Fn = cast(GV);
+Fn->addFnAttr("patchable-function", "ms-hotpatch");
+ }
}
}
};
Index: include/clang/Basic/AttrDocs.td
===
--- include/clang/Basic/AttrDocs.td
+++ include/clang/Basic/AttrDocs.td
@@ -494,6 +494,19 @@
}];
}
+def MSHookPrologueDocs : Documentation {
+ let Category = DocCatFunction;
+ let Content = [{
+The ``ms_hook_prologue`` attribute marks a function as "hotpatchable" according
+to conventions used on Windows. Specifically, enough space will be ensured
+in the prologue for a short jump, and an architecturally dependently sized
+patch space will be reserved prior to the entry point. See the documentation
+for the `/HOTPATCH`_ switch on MSDN.
+
+.. _`/HOTPATCH/: https://msdn.microsoft.com/en-us/library/ms173507.aspx
+ }];
+}
+
def NoDebugDocs : Documentation {
let Category = DocCatVariable;
let Content = [{
Index: include/clang/Basic/Attr.td
===
--- include/clang/Basic/Attr.td
+++ include/clang/Basic/Attr.td
@@ -2029,6 +2029,12 @@
// Microsoft-related attributes
+def MSHookPrologue : InheritableAttr {
+ let Spellings = [GCC<"ms_hook_prologue">];
+ let Subjects = SubjectList<[Function]>;
+ let Documentation = [MSHookPrologueDocs];
+}
+
def MSNoVTable : InheritableAttr, TargetSpecificAttr {
let Spellings = [Declspec<"novtable">];
let Subjects = SubjectList<[CXXRecord]>;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19909: [Attr] Add support for the `ms_hook_prologue` attribute.
aaron.ballman added a comment.
Generally, I think this should make use of the patchable function functionality
from http://reviews.llvm.org/D19046 when lowering, but I do have some comments
on the attribute itself as well.
Comment at: include/clang/Basic/Attr.td:2032
@@ -2031,1 +2031,3 @@
+def MSHookPrologue : InheritableAttr {
+ let Spellings = [GCC<"ms_hook_prologue">];
Does this attribute appertain to all targets, or only targets that MSVC
supports?
Comment at: include/clang/Basic/Attr.td:2035
@@ +2034,3 @@
+ let Subjects = SubjectList<[Function]>;
+ let Documentation = [Undocumented];
+}
Please, no undocumented new attributes.
http://reviews.llvm.org/D19909
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
