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

[Min-Yih Hsu via Phabricator via cfe-commits]

myhsu added inline comments.


================
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 ||
----------------
aaron.ballman wrote:
> 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.
The idea was to use our new RTD CC whenever `-mrtd` is given to either the driver or the frontend. Due to some historical reasons `-mrtd` has been taken by i386 to specify stdcall. Specifically, DefaultCallingConvention will be set to `DCC_StdCall` when `-mrtd` is present. My original approach was to reuse this workflow and the `DCC_StdCall`, then translate to either `CC_X86StdCall` or `CC_M68kRTD` later in ASTContext.

Since there are many concerns on reusing DCC_StdCall, I will create another DCC enum value instead.


================
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);
+}
----------------
aaron.ballman wrote:
> 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?
> 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?

I'm not sure if I understand you correctly, but this diagnostic is emitted if the CC does not support variadic function call. 


================
Comment at: clang/test/Sema/m68k-mrtd.c:45
+extern void (*d)(int, ...);
+__attribute__((m68k_rtd)) extern void (*d)(int, ...);
----------------
aaron.ballman wrote:
> 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?
> Function without a prototype

I thought the first check was testing function without a prototype.

> What should happen for C++ and things like member functions?

I believe we don't have any special handling for C++.

I addressed rest of the bullet items you mentioned, please take a look.


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

https://reviews.llvm.org/D149867