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