If some platforms do not support an attribute, we should exclude the platform

[HLJ2009]

Summary

I tested the alias attribute on my own Apple laptop (Target: x86_64-apple-darwin17.5.0). First, I use has_attribute to test that the alias is usable or not. The test code is as follows:
#include <stdio.h>
void print() {
#if has_attribute(alias)

printf("has attribute");

#else

printf("has not attribute");

#endif
}
int main() {

print();
return 0;

}
Compiled using clang, the output result is has attribute, but when i use the following code to verify
#include <stdio.h>
int oldname = 1;
extern int newname attribute((alias("oldname"))); // declaration
void foo(void)
{

printf("newname = %d\n", newname); // prints 1

}
int main() {

foo();
return 0;

}
It told me alias.c:3:35: error: aliases are not supported on darwin.so we should exclude the platform.

[lebedev.ri]

1. Please always upload all patches with full context.
2. tests?

[aaron.ballman]

Thank you for working on this patch -- be sure to add tests that ensure the attribute is properly prohibited on the expected targets.

include/clang/Basic/Attr.td

299–300

How about: If set to false, indicates the attribute is supported only on the given target. If set to true, indicates the attribute is supported on all targets except the given target.

324–325

This comment is a bit too specific, I'd probably drop the comment entirely as the code is sufficiently clear.

326

I think we'll want to pattern the names like TargetNotFoo.

One other thing is: this isn't really Darwin -- for instance, there's ARM and AArch64 variants, not just x86-based targets.

327

I would expect the OS would be named "Darwin" and not "MacOSX" given the name of the target. Which flavor should be used, or should both be used?

utils/TableGen/ClangAttrEmitter.cpp

2789

The formatting is off here -- there should be a space after the if. I would recommend running your patch through clang-format (https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting).

[rsmith]

include/clang/Basic/Attr.td

566–567

Is this really specific to Darwin? I would expect it instead to be specific to the ObjectFormat. And I think listing the object formats that *do* support aliases seems reasonable (as there's only three of them to list -- assuming that WAsm actually supports aliases, which I don't know -- and we'd probably want future object file formats to opt in rather than opting out).

[HLJ2009]

include/clang/Basic/Attr.td

327

hi,Aaron

First, I set OSType to Darwin,but it does not work on my own machine. so I set OSType to MacOSX,and it is ok.Yes, I thought about adding two at the same time, but this needs to modify list<string> OSes to list<string> OSes[].

[HLJ2009]

include/clang/Basic/Attr.td

566–567

Hi,
   If using this program, we need to define a support alias platform, is not it? This platform needs to include the ArchType in the Triple.h file for the most part, and we need to set the ObjectFormatType to COFF, ELF, and Wasm. But we only want to exclude the few unsupported platforms. If my understanding is not right, please told me.

[HLJ2009]

listing the object formats that *do* support aliases seems reasonable

[aaron.ballman]

@rsmith -- do the object file formats listed look correct to you?

include/clang/Basic/Attr.td

322

Did you verify that Wasm supports the alias attribute? If it is supported, it might be nice to add a test to CodeGen/alias.c to demonstrate it. Similar for COFF.

test/Sema/attr-alias-has.c

5 ↗

(On Diff #147773)

I'd like to see a test that the "attribute not supported on target" diagnostic is being generated. I'd recommend something along these lines:

void g() {}
void f() __attribute__((alias("g")));
#if !__has_attribute(alias)
// expected-error@-2{{expected diagnostic text}}
#else
// expected-no-diagnostics
#endif

[rsmith]

In D46805#1113358, @aaron.ballman wrote:

@rsmith -- do the object file formats listed look correct to you?

They look at least plausible. We should be able to test whether LLVM can actually emit aliases on each of these targets easily enough...

However, I get this error for any WAsm compilation I try:

fatal error: error in backend: section size does not fit in a uint32_t

... so I have no idea if aliases are/will be supported there. Perhaps @sunfish can tell us :)

[sunfish]

In D46805#1115681, @rsmith wrote:

In D46805#1113358, @aaron.ballman wrote:

@rsmith -- do the object file formats listed look correct to you?

They look at least plausible. We should be able to test whether LLVM can actually emit aliases on each of these targets easily enough...

However, I get this error for any WAsm compilation I try:

fatal error: error in backend: section size does not fit in a uint32_t

... so I have no idea if aliases are/will be supported there. Perhaps @sunfish can tell us :)

Yes, they are intended to be supported. It sounds like you found a bug, though I've not been able to reproduce it in simple tests.

[HLJ2009]

In D46805#1117091, @sunfish wrote:

In D46805#1115681, @rsmith wrote:

In D46805#1113358, @aaron.ballman wrote:

@rsmith -- do the object file formats listed look correct to you?

They look at least plausible. We should be able to test whether LLVM can actually emit aliases on each of these targets easily enough...

However, I get this error for any WAsm compilation I try:

fatal error: error in backend: section size does not fit in a uint32_t

... so I have no idea if aliases are/will be supported there. Perhaps @sunfish can tell us :)

Yes, they are intended to be supported. It sounds like you found a bug, though I've not been able to reproduce it in simple tests.

include/clang/Basic/Attr.td

322

Yes, I used the following command line to test my test file.
clang -cc1 -triple wasm32-unknown-unknown -fsyntax-only -verify attr-alias-has.c
clang -cc1 -triple wasm64-unknown-unknown -fsyntax-only -verify attr-alias-has.c
The test result is ok.
ok, I will update it.

test/Sema/attr-alias-has.c

5 ↗

(On Diff #147773)

ok, I will update it.

[HLJ2009]

update test case.

[aaron.ballman]

LGTM aside from a nit with one of the tests. Once you've updated the patch and verified that check-clang passes all tests, I can commit for you next week when I'm back from meetings (unless someone else gets to it before me).

include/clang/Basic/Attr.td

322

That's perhaps not the "support" I was talking about. I didn't mean "are you sure the target spec on the attribute is doing what you expect", but "are you sure that alias works and has the expected semantics when the target is Wasm"?

However, @sunfish already said it's intended to be supported in Wasm, so I think that's fine.

564

Now that we're touching this attribute so that it no longer is available on some targets, it would be good to consider adding documentation for the attribute. That documentation isn't your responsibility and doesn't need to be part of this patch, but if it was something you wanted to tackle in a follow-up, that'd be appreciated (but not required).

test/Sema/attr-alias-has.c

13 ↗

(On Diff #149665)

Please add a newline to the end of the file.

include/clang/Basic/Attr.td

   // Specifies Object Formats for which the target applies, based off the
   // ObjectFormatType enumeration in Triple.h
   list<string> ObjectFormats;
+  // It indicates that a certain attribute is not supported on a specific
+  // platform, turn on support for this attribute by default
+  bit Negated = 0;
 }
 
 class TargetArch<list<string> arches> : TargetSpec {

   let ObjectFormats = ["ELF"];
 }
 
+// We do not support the alias attribute on Apple platforms,
+// so I define a platform other than the Apple platform.
+def TargetDarwinNegative : TargetArch<["x86", "x86_64"]> {
+  let OSes = ["MacOSX"];
+  let Negated = 1;
+}
+
 // Attribute subject match rules that are used for #pragma clang attribute.
 //
 // A instance of AttrSubjectMatcherRule represents an individual match rule.

   let Documentation = [Undocumented];
 }
 
-def Alias : Attr {
+// We do not support alias attribute on Apple platform, so we exclude the platform.
+def Alias : Attr, TargetSpecificAttr<TargetDarwinNegative> {
   let Spellings = [GCC<"alias">];
   let Args = [StringArgument<"Aliasee">];
   let Subjects = SubjectList<[Function, GlobalVar], ErrorDiag>;

utils/TableGen/ClangAttrEmitter.cpp

   GenerateTargetSpecificAttrCheck(R, Test, FnName, "OSes", "T.getOS()",
                                   "llvm::Triple::");
 
+  // If the Negated is 1 in specific, it indicates that the attribute
+  // is not supported on this platform
+  if(R->getValueAsBit("Negated"))
+    Test = "!(" + Test + ")";
+
   // If one or more CXX ABIs are specified, check those as well.
   GenerateTargetSpecificAttrCheck(R, Test, FnName, "CXXABIs",
                                   "Target.getCXXABI().getKind()",