Page MenuHomePhabricator

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

Authored by HLJ2009 on May 13 2018, 7:25 AM.

Details

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.

Diff Detail

Event Timeline

HLJ2009 created this revision.May 13 2018, 7:25 AM
  1. Please always upload all patches with full context.
  2. tests?

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 added a subscriber: rsmith.May 13 2018, 2:39 PM
rsmith added inline comments.
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 marked 4 inline comments as done.May 14 2018, 5:29 AM
HLJ2009 added inline comments.
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 added inline comments.May 14 2018, 7:57 AM
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 updated this revision to Diff 147773.May 21 2018, 5:43 AM

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

HLJ2009 marked 4 inline comments as done.May 21 2018, 6:10 PM

@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 -- 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 :)

@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.

@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 updated this revision to Diff 149665.Jun 3 2018, 7:06 PM

update test case.

aaron.ballman accepted this revision.Jun 5 2018, 1:48 AM

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.

This revision is now accepted and ready to land.Jun 5 2018, 1:48 AM