Page MenuHomePhabricator

Add support for MS qualifiers __ptr32, __ptr64, __sptr, __uptr.
Needs ReviewPublic

Authored by akhuang on Aug 27 2019, 1:03 PM.

Details

Reviewers
rnk
rsmith
Summary

Previously, these qualifiers were being parsed but otherwise ignored.
This change makes it so that an address space is added to specify whether the pointer is
32-bit or 64-bit and whether it is sign extended or zero extended.

In the backend, the address space casts are lowered to the corresponding
sign/zero extension or truncation.

The data layout for the address spaces was changed in https://reviews.llvm.org/D64931

Related to https://bugs.llvm.org/show_bug.cgi?id=42359

Event Timeline

akhuang created this revision.Aug 27 2019, 1:03 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 27 2019, 1:03 PM
rnk added a comment.Aug 27 2019, 2:59 PM

This change needs a clang CodeGen test to show that we generate IR with __ptr32 / __ptr64 in the correct places. We should already have some semantic tests, but we may need more.

clang/lib/AST/MicrosoftMangle.cpp
1882

Hm, we should actually mangle these as they do. See the FIXME comment above the definition of PointersAre64Bit. If you look at the uses of PointersAre64Bit, you should be able to find the places where you need to check if a pointer type is either in the explicit 64-bit address space or in the default address space for a 64-bit target. That check sounds like a good helper function.

clang/lib/Sema/SemaOverload.cpp
2874

This probably deserves a comment. It looks like this lets you do this:

void foo(int *__ptr64);
void foo(int *p) { } // assume x64 target

... without having the compiler think it's creating an overload.

Separately, MSVC doesn't permit __ptr32/__ptr64 overloads. Is it possible to implement that here as well?

llvm/test/CodeGen/X86/mixed-ptr-sizes.ll
12

Do you need use_foo? I think f is a parameter, so the compiler can't remove any stores to it, even without a call to use it. You should be able to simplify the test to skip these calls.

31

If you compile this code as plain C code, then it won't have as much name mangling, which makes the .ll file more readable.

akhuang updated this revision to Diff 217940.Aug 29 2019, 10:59 AM
akhuang marked 4 inline comments as done.
  • Test that codegen adds the correct address spaces
  • Modify microsoft mangling to match microsoft mangling.
  • add comment for overloading
akhuang marked 2 inline comments as done.Aug 29 2019, 11:02 AM
akhuang added inline comments.
clang/lib/AST/MicrosoftMangle.cpp
1882

Added a function to check for 64-bit address space / default 64-bit pointer. I also changed it so that the pointer size address spaces mangle as normal types and not address space types.

clang/lib/Sema/SemaOverload.cpp
2874

Comment added.

Although I think that for

void foo(int *__ptr64);
void foo(int *p) { } // assume x64 target

this code doesn't affect the overload because int * __ptr64 and int * are already the same type?

As far as I can tell this also doesn't allow __ptr32/__ptr64 overloads, because it counts them as the same type. It gives a "conflicting types" error instead of a "redefintion" error though, so I'll look into that.

llvm/test/CodeGen/X86/mixed-ptr-sizes.ll
12

Deleted use_foo, thanks.

31

done

akhuang updated this revision to Diff 218795.Sep 4 2019, 3:07 PM
  • Change existing tests so they still pass
  • Fix so that it parses all the __ptr32/64 attributes instead of just the last one
majnemer added inline comments.
clang/lib/AST/MicrosoftMangle.cpp
1718

defulat -> default

brooks added a subscriber: brooks.Sep 10 2019, 1:45 AM

@akhuang
Any update on this? I'd love to see this upstreamed. I'd offer to help, but I'm not well versed in clang.

FWIW, I also found a small bug when using this patchset:

void func1(void * __ptr32 test) { 
}
int main() {
	func1(0);
	return 0;
}

gives

fatal error: error in backend: Cannot emit physreg copy instruction

This only happens when passing a 0. If you pass 1, for example, it gives a warning (because of no cast) but compiles.
Tested against MSCV, which compiles the code without problems.

@DarkShadow44 Thanks for finding this bug! I haven't had time to look at this for a while but will start working on it soon.

@akhuang Thanks for looking into this!

I've found something else, I've written a small dumper to demonstrate. Pardon the long comment please.

Program:

typedef int* __ptr32 PINT32;
typedef int* PINT64;

struct s1
{
	PINT32 i32;
	PINT64 i64;
};

AST:

Kind: 'TranslationUnit(300)', Type: '', Name: '/home/fabian/Programming/Wine/wine-git/tools/test.c'
        Kind: 'TypedefDecl(20)', Type: '__ptr32_sptr int *', Name: 'PINT32'
        Kind: 'TypedefDecl(20)', Type: 'PINT64', Name: 'PINT64'
        Kind: 'StructDecl(2)', Type: 'struct s1', Name: 's1'
                Kind: 'FieldDecl(6)', Type: '__ptr32_sptr int *', Name: 'i32'
                        Kind: 'TypeRef(43)', Type: '__ptr32_sptr int *', Name: 'PINT32'
                Kind: 'FieldDecl(6)', Type: 'PINT64', Name: 'i64'
                        Kind: 'TypeRef(43)', Type: 'PINT64', Name: 'PINT64'

Note the difference between

Type: '__ptr32_sptr int *', Name: 'PINT32'
Type: 'PINT64', Name: 'PINT64'

3 Issues I have here:

  • __ptr32_sptr doesn't exist and seems wrong
  • The __ptr32 part should be right of the asterisk
  • Why does PINT64 have PINT64 as type, but PINT32 not PINT32?

I'm not sure if this is/should still be part of this patchset or even is an issue, just pointing out inconsistencies I noticed - hope I'm not a bother.

Dumper attached, for convenience.

I split off the backend changes into a separate patch -> https://reviews.llvm.org/D69639
The issue with passing 0 for a pointer should be fixed there.

@DarkShadow44
Some of the differences come from the fact that we're implementing the mixed pointer sizes with three address spaces, which get printed out as __ptr32_sptr, __ptr32_uptr, and __ptr64. So they are intended but maybe also confusing.

__ptr32_sptr doesn't exist and seems wrong

In this case it picks the address space __ptr32_sptr because it does sign extension by default.

The __ptr32 part should be right of the asterisk

I think this happens because address spaces are attached to the pointee type and not the pointer.

Why does PINT64 have PINT64 as type, but PINT32 not PINT32?

I'm not sure why this happens, but I can look into it later. It makes sense that PINT64 would work as it did before because on a 64-bit system adding __ptr64 doesn't do anything.