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@+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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
