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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

In D149867#4544489 <https://reviews.llvm.org/D149867#4544489>, @myhsu wrote:

> Sorry I was busy on my phd defense (which I passed!) in the past month. I'll get back to this next week.

Congratulations!! 🎉

LGTM!



================
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:
> > 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.
> I understand what you meant, but the standard only says that using implicit declared identifier is as if `extern int ident();` appears in the source code. My interpretation is that in this case since the very source code is compiled with `-mrtd`, every functions use m68k_rtd rather than C calling convention. 
> 
> Another example is stdcall: i386 Clang emits a similar warning ("function with no prototype cannot use the stdcall calling convention") when `-mrtd` is used (to set default CC to stdcall) on the same snippet. Should `bar` was not called with stdcall under the influence of `-mrtd`, the aforementioned message would not be emitted in the first place.
> i386 GCC also calls `bar` with stdcall when `-mrtd` is given (though no warning message is emitted).
Ahhh okay, this makes more sense now -- this matches other ways in which we set the default calling convention, it's just a bit more hidden from view. Here's a more obvious example: https://godbolt.org/z/sboxf83s6


================
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:
> > 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?)
> 
> > ```
> > __attribute__((m68k_rtd)) void bar(a, b) int a, b; {} // Should error
> > ```
> 
> Right now Clang only gives a warning about potential behavior change after C23, rather than an error for this kind of syntax. Because it seems like Clang doesn't check this situation against unsupported calling convention. I'm not sure if we should throw the same error as `__attribute__((m68k_rtd)) void foo()`.
> 
> > 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?)
> 
> I don't think it's worth it to support m68k with Windows ABI as this combination never existed (and unlikely to happen in the future)
> Right now Clang only gives a warning about potential behavior change after C23, rather than an error for this kind of syntax. Because it seems like Clang doesn't check this situation against unsupported calling convention. I'm not sure if we should throw the same error as __attribute__((m68k_rtd)) void foo().

It looks like this is an existing Clang bug: https://godbolt.org/z/WcYbcEhde

> I don't think it's worth it to support m68k with Windows ABI as this combination never existed (and unlikely to happen in the future)

SGTM!


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

https://reviews.llvm.org/D149867