myhsu added inline comments.
================
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:
> 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).
================
Comment at: clang/test/Sema/m68k-mrtd.c:45
+extern void (*d)(int, ...);
+__attribute__((m68k_rtd)) extern void (*d)(int, ...);
----------------
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)
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
