[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.

Sorry for the long delay in review on this, it slipped through the cracks for me.



================
Comment at: clang/lib/AST/TypePrinter.cpp:1861
     break;
+  case attr::M68kRTD: OS << "m68k_rtd"; break;
   case attr::NoDeref:
----------------



================
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);
+}
----------------
myhsu wrote:
> 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. 
`bar` is not declared, in C89 this causes an implicit function declaration to be generated with the signature: `extern int ident();` What surprised me is that we seem to be generating something more like this instead: `__attribute__((m68k_rtd)) int ident();` despite that not being valid.


================
Comment at: clang/test/Sema/m68k-mrtd.c:45
+extern void (*d)(int, ...);
+__attribute__((m68k_rtd)) extern void (*d)(int, ...);
----------------
myhsu wrote:
> 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.
>> Function without a prototype
> I thought the first check was testing function without a prototype.

Nope, I meant something more like this:
```
__attribute__((m68k_rtd)) void foo(); // Should error
__attribute__((m68k_rtd)) void bar(a, b) int a, b; {} // Should error
```

>> What should happen for C++ and things like member functions?
> I believe we don't have any special handling for C++.

Test coverage should still exist to ensure the behavior is what you'd expect when adding the calling convention to a member function and a lambda, for example. (Both Sema and CodeGen tests for this)

Also missing coverage for the changes to `-fdefault-calling-conv=`

I'm also still wondering whether there's a way to use m68k with a Windows ABI triple (basically, do we need changes in MicrosoftMangle.cpp?)


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

https://reviews.llvm.org/D149867