[PATCH] D149867: [Clang][M68k] Add Clang support for the new M68k_RTD CC

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

This should also come with a release note for the changes.



================
Comment at: clang/lib/AST/ASTContext.cpp:11998-11999
+          return CC_X86StdCall;
+        else if (T.getArch() == llvm::Triple::m68k)
+          return CC_M68kRTD;
+      }
----------------
This looks wrong to me -- if the language option is calling for stdcall, saying "here's m68k_rtd instead" does not match caller expectations.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:559
+    emitError |= DefaultCC == LangOptions::DCC_StdCall &&
+                 Arch != llvm::Triple::m68k && Arch != llvm::Triple::x86;
     emitError |= (DefaultCC == LangOptions::DCC_VectorCall ||
----------------
Maybe it's too early in the morning for me to be thinking clearly, but this is wrong for m68k, isn't it? If the default calling convention is stdcall and the architecture is m68k, we want to emit the error, don't we?

I don't see test coverage for the change.


================
Comment at: clang/lib/Sema/SemaType.cpp:7960-7961
     if (FnP && FnP->isVariadic()) {
-      // stdcall and fastcall are ignored with a warning for GCC and MS
-      // compatibility.
-      if (CC == CC_X86StdCall || CC == CC_X86FastCall)
+      // stdcall, fastcall, and m68k's RTD are ignored with a warning for GCC
+      // and MS compatibility.
+      if (CC == CC_X86StdCall || CC == CC_X86FastCall || CC == CC_M68kRTD)
----------------
Missing test coverage, but also, MSVC supports m68k RTD? I would assume we'd error on this situation given how we document the attribute as not supporting this.


================
Comment at: clang/test/CodeGen/mrtd.c:4
 
-// CHECK: mrtd.c:10:3: warning: function with no prototype cannot use the stdcall calling convention
+// CHECK: mrtd.c:13:3: warning: function with no prototype cannot use the stdcall calling convention
 
----------------
myhsu wrote:
> jrtc27 wrote:
> > Ew... this should be using -verify and `// expected-warning {{...}}`, then the line number is relative to the comment's location. Or, really, it should be in the Sema test...
> > 
> > Plus is it correct to call this stdcall on m68k? The GCC manpage only mentions it in the x86 option description, not the m68k one.
> Now this check is moved to `test/Sema/m68k-mrtd.c`
This check should be removed here -- we don't typically test diagnostic behavior from codegen tests unless the diagnostic is only emitted during codegen.


================
Comment at: clang/test/Sema/m68k-mrtd.c:4-9
+#ifdef MRTD
+// expected-error at +3 {{function with no prototype cannot use the m68k_rtd calling convention}}
+#endif
+void foo(int arg) {
+  bar(arg);
+}
----------------
A better way to do this is to use `-verify=mrtd` on the line enabling rtd, and using `// rtd-error {{whatever}}` on the line being diagnosed. (Same comment applies throughout the file.)

Huh, I was unaware that implicit function declarations are using something other than the default calling convention (which is C, not m68k_rtd). Is this intentional?


================
Comment at: clang/test/Sema/m68k-mrtd.c:45
+extern void (*d)(int, ...);
+__attribute__((m68k_rtd)) extern void (*d)(int, ...);
----------------
Missing tests for:

* Function without a prototype
* Applying the attribute to a non-function
* Providing arguments to the attribute
* What should happen for C++ and things like member functions?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149867/new/

https://reviews.llvm.org/D149867