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