Page MenuHomePhabricator

[MSVC] PR27132: Proper mangling for __unaligned qualifier
ClosedPublic

Authored by andreybokhanko on Mar 30 2016, 7:10 AM.

Details

Summary

This patch implements proper mangling for MS-specific "unaligned" qualifier. The mangling part itself is tiny; most of the patch is required to convert "unaligned" from an ignored qualifier to a non-ignored one.

See PR27132 for an example of code that mangles incorrectly at the moment.

Yours,

Andrey

Software Engineer
Intel Compiler Team

Diff Detail

Repository
rL LLVM

Event Timeline

andreybokhanko retitled this revision from to [MSVC] PR27132: Proper mangling for __unaligned qualifier.
andreybokhanko updated this object.
andreybokhanko added reviewers: rnk, majnemer.
andreybokhanko added a subscriber: cfe-commits.

I'd like to see some Sema tests for sanity checking; like applying __unaligned to a non-pointer type, to a declaration, when -fno-ms-extensions is enabled, etc.

include/clang/Basic/Attr.td
2080 ↗(On Diff #52055)

This is possibly missing let LangOpts = [MicrosoftExt];

include/clang/Basic/AttrDocs.td
1964 ↗(On Diff #52055)

"an" unaligned address.

lib/Sema/SemaType.cpp
5555 ↗(On Diff #52055)

This appears to be untested, but really should have a test case to ensure we don't regress later.

rnk added inline comments.Mar 30 2016, 9:17 AM
include/clang/Basic/AttrDocs.td
1970 ↗(On Diff #52055)

I don't think "IPF targets" will be understood by readers to mean "Itanium targets". I'd just drop everything before the semicolon. At some point, we might want to add support __unaligned __m128*. Today we'll copy that with movaps.

lib/Sema/SemaType.cpp
5555 ↗(On Diff #52055)

+1, although I recall that someone else from Intel wanted to make sure this test case works:

struct A { void g() __unaligned {} };

I could take it or leave it. I don't think that rejecting this program will break very much real code.

majnemer edited edge metadata.Mar 30 2016, 10:13 AM

I didn't implement a mangling for __unaligned because our implementation of it is broken.
It should not be modeled as an attribute, it should be modeled as a qualifier because it is possible to overload on it.

andreybokhanko edited edge metadata.

Resolved [some of] comments made by Aaron and Reid.

andreybokhanko marked 3 inline comments as done.Mar 31 2016, 8:04 AM

Aaron, Reid, David, thank you for the review!

I resolved some of your comments. As for

I'd like to see some Sema tests for sanity checking; like applying __unaligned to a non-pointer type, to a declaration, when -fno-ms-extensions is enabled, etc.

Do we want to accept unaligned for non-pointer types, as MS compiler does? Their doc (https://msdn.microsoft.com/en-us/library/ms177389.aspx) only mentions pointers, but cl.exe also accepts non-pointers with unaligned, with no visible effect to generated code (including mangling). Clang currently does accept this, so starting to issue an error would be a regression.

I didn't implement a mangling for __unaligned because our implementation of it is broken.
It should not be modeled as an attribute, it should be modeled as a qualifier because it is possible to overload on it.

I see.

Well, now we have not quite compatible implementation + incorrect mangling. My patch fixes mangling part. IMHO, implementation part should be fixed in a separate patch.

Andrey

Aaron, Reid, David, thank you for the review!

I resolved some of your comments. As for

I'd like to see some Sema tests for sanity checking; like applying __unaligned to a non-pointer type, to a declaration, when -fno-ms-extensions is enabled, etc.

Do we want to accept unaligned for non-pointer types, as MS compiler does? Their doc (https://msdn.microsoft.com/en-us/library/ms177389.aspx) only mentions pointers, but cl.exe also accepts non-pointers with unaligned, with no visible effect to generated code (including mangling). Clang currently does accept this, so starting to issue an error would be a regression.

Regression is a bit of a question mark, to me depending on the diagnostic. I think warning the user "this has absolutely no effect" is a reasonable diagnostic for that situation -- the user wrote something, possibly expecting it to have an effect, and it turns out that it does absolutely nothing (including in the compiler that implements the language extension). If MSVC were to ever add semantic effect in those cases (diverging from what Clang implements), the diagnostic becomes even more important for Clang users. So I think it's fine to accept __unaligned for non-pointer types, but issue an "attribute ignored" warning diagnostic.

Added Sema test, as per Aaron's and Reid's request.

Regression is a bit of a question mark, to me depending on the diagnostic. I think warning the user "this has absolutely no effect" is a reasonable diagnostic for that situation -- the user wrote something, possibly expecting it to have an effect, and it turns out that it does absolutely nothing (including in the compiler that implements the language extension). If MSVC were to ever add semantic effect in those cases (diverging from what Clang implements), the diagnostic becomes even more important for Clang users. So I think it's fine to accept __unaligned for non-pointer types, but issue an "attribute ignored" warning diagnostic.

As David wrote, __unaligned is a qualifier in MSVC, so MS accepts the following:

__unaligned int *p;

as a correct usage (and does mangling for __unaligned).

We model it as an attribute, so we create a new AttributedType for int, not for the pointer. This is OK, since our mangling code takes PointeeType and checks presence of the attribute. Unfortunately, this means that we can't issue warnings, as it's impossible (to the best of my knowledge) to distinguish between

unaligned int *p;
unaligned int p;

in processTypeAttrs function.

As I said before, the purpose of this patch is to implement correct mangling (and thus, improve object level compatibility with MSVC), not to provide a fully correct implementation of __unaligned.

Another alternative is to model __unaligned as a qualifier, but this would require addition of an extra field to TypeBitfields. Do we want to do this for an ignored qualifier? I don't see any practical purpose.

Andrey

Regression is a bit of a question mark, to me depending on the diagnostic. I think warning the user "this has absolutely no effect" is a reasonable diagnostic for that situation -- the user wrote something, possibly expecting it to have an effect, and it turns out that it does absolutely nothing (including in the compiler that implements the language extension). If MSVC were to ever add semantic effect in those cases (diverging from what Clang implements), the diagnostic becomes even more important for Clang users. So I think it's fine to accept __unaligned for non-pointer types, but issue an "attribute ignored" warning diagnostic.

As David wrote, __unaligned is a qualifier in MSVC, so MS accepts the following:

__unaligned int *p;

as a correct usage (and does mangling for __unaligned).

We model it as an attribute, so we create a new AttributedType for int, not for the pointer. This is OK, since our mangling code takes PointeeType and checks presence of the attribute. Unfortunately, this means that we can't issue warnings, as it's impossible (to the best of my knowledge) to distinguish between

unaligned int *p;
unaligned int p;

in processTypeAttrs function.

As I said before, the purpose of this patch is to implement correct mangling (and thus, improve object level compatibility with MSVC), not to provide a fully correct implementation of __unaligned.

Another alternative is to model __unaligned as a qualifier, but this would require addition of an extra field to TypeBitfields.

That's OK. We have plenty of bits in the "address_space" field to steal from.

Do we want to do this for an ignored qualifier? I don't see any practical purpose.

It's not ignored for Windows on ARM.

Andrey

Do we want to do this for an ignored qualifier? I don't see any practical purpose.

It's not ignored for Windows on ARM.

Hmmm... MS documentation explicitly mentions IPF only:

When you declare a pointer with the __unaligned modifier, the compiler assumes that the pointer addresses data that is not aligned. Consequently, for an application that targets an Itanium Processor Family (IPF) computer, the compiler generates code that reads the unaligned data one byte at a time.

OK, I will implement _unaligned as a qualifier, if you think this is the right way to go. Note, though, that it will remain ignored, as there is no support for _unaligned in LLVM IR.

Andrey

This patch implements "__unaligned" as a qualifier (as per David Majnemer's request).

Most of the changes are miscellaneous small stuff one has to add when adding a new qualifier.

Other notable things are:

  • One bit is stolen from "AddressSpace" to store __unaligned. This resulted in changes in OpenCL code and tests, that implicitly assumed that AddressSpace size is 24 bits.
  • Sema checks added. It verifies that __unaligned can be used with non-pointers, for overloading and that it doesn't get recognized without -fms-compatibility option.
  • Proper mangling (along with tests) for unaligned added. Note that non-pointer unaligned args don't affect mangling.

Andrey

rnk added inline comments.Apr 8 2016, 10:21 AM
test/SemaCXX/MicrosoftExtensions.cpp
89 ↗(On Diff #53022)

Surely we can come up with some tougher overloading test cases. I noticed MSVC generates a warning when converting from '__unaligned T*' to 'T*' with this code:

extern "C" int puts(const char *);
void f(__unaligned int *p) {
  puts("unaligned");
}
void f(int *p) {
  puts("aligned");
}
void g(int *p) {
  puts("g");
}
int main() {
  int *p = 0;
  __unaligned int *q = 0;
  f(p);
  f(q);
  g(p);
  g(q); // C4090
}
  1. Added more comprehensive overloading tests
  2. Added a warning (in C mode) / error (in C++ mode) on unaligned to non-unaligned type conversion

Andrey

andreybokhanko marked an inline comment as done.Apr 12 2016, 7:06 PM
andreybokhanko added inline comments.
test/SemaCXX/MicrosoftExtensions.cpp
89 ↗(On Diff #53509)

Reid, thanks for looking into this patch!

I added more comprehensive overloading tests.

As for printing a warning, I added it -- in C mode. In C++ mode we don't allow types conversion that loses qualifiers. I do the same for __unaligned. If you want me to implement printing a warning in C++ mode as well (as MS compiler does), I would appreciate any hints on where the best place to do it. Putting it directly into code that verifies qualifiers compatibility seems to be odd.

Andrey

rnk accepted this revision.Apr 13 2016, 1:05 PM
rnk edited edge metadata.

Thanks! This looks great.

test/SemaCXX/MicrosoftExtensions.cpp
89 ↗(On Diff #53509)

I think we can keep it as you have it for now. We can wait and see if users complain about the C++ mode error.

This revision is now accepted and ready to land.Apr 13 2016, 1:05 PM
This revision was automatically updated to reflect the committed changes.
andreybokhanko marked an inline comment as done.Apr 15 2016, 1:09 AM

Reid, thank you!

Yours,
Andrey