Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clang errors while semantic checking with incomplete class member pointer type #12442

Closed
nico opened this issue Feb 23, 2012 · 23 comments
Closed
Labels
bugzilla Issues migrated from bugzilla c++

Comments

@nico
Copy link
Contributor

nico commented Feb 23, 2012

Bugzilla Link 12070
Resolution FIXED
Resolved on Apr 17, 2013 14:14
Version unspecified
OS All
Blocks llvm/llvm-bugzilla-archive#13707
CC @DougGregor,@tritao,@rnk,@timurrrr,@ftynse

Extended Description

$ cat test.cc
class A {
public:
typedef int (A::*Foo)(int);
};

hummer:src thakis$ ~/src/llvm-svn/Release+Asserts/bin/clang -c test.cc -Xclang -cxx-abi -Xclang microsoft
test.cc:3:19: error: incomplete type 'A' where a complete type is required
typedef int (A::*Foo)(int);
^
test.cc:1:7: note: definition of 'A' is not complete until the closing '}'
class A {
^
1 error generated.

Works fine without -cxx-abi.

@nico
Copy link
Contributor Author

nico commented Feb 23, 2012

This is explained by this comment in SemaType.cpp:

// In the Microsoft ABI, the class is allowed to be an incomplete
// type. In such cases, the compiler makes a worst-case assumption.
// We make no such assumption right now, so emit an error if the
// class isn't a complete type.
if (Context.getTargetInfo().getCXXABI() == CXXABI_Microsoft &&
RequireCompleteType(Loc, Class, diag::err_incomplete_type))
return QualType();

@nico
Copy link
Contributor Author

nico commented Feb 23, 2012

@tritao
Copy link
Mannequin

tritao mannequin commented Jun 23, 2012

MS driver improvements

@tritao
Copy link
Mannequin

tritao mannequin commented Jun 23, 2012

Sorry for the noise, attached patch to wrong bug.

@timurrrr
Copy link
Contributor

Hi Charles,

Can you explain the background behind r111118?
Why don't we allow this kind of things in the MS ABI ?

@llvmbot
Copy link
Member

llvmbot commented Jun 26, 2012

Hi Charles,

Can you explain the background behind r111118?
Why don't we allow this kind of things in the MS ABI ?
If you'll direct your attention to the MicrosoftCXXABI::getMemberPointerSize() method in lib/AST/MicrosoftCXXABI.cpp, you'll notice that we refer back to the target class several times, in order to determine its inheritance type--i.e. the ones you can explicitly declare with the __*_inheritance attributes. We can't do that if the class is incomplete.

I guess we could fix it by checking if the CXXRecordDecl is incomplete, and return 3 ptrdiff_t's (the worst-case scenario) if it is. Then we can revert r111118.

Also, the getMemberPointerSize() method needs to be fixed to obey the '__*_inheritance' attributes that João added. I'm assuming that's what they're for. I'll be happy to commit a patch for that :). (I'm kinda busy right now, though, so actually writing the patch is up to you guys.)

@tritao
Copy link
Mannequin

tritao mannequin commented Jun 26, 2012

Thanks for the explanation. That sounds doable.

I can take a look into it.

@tritao
Copy link
Mannequin

tritao mannequin commented Jun 26, 2012

Member pointer patch

@tritao
Copy link
Mannequin

tritao mannequin commented Jun 26, 2012

I did not test this patch yet, just sharing if someone wants to work on it.

@timurrrr
Copy link
Contributor

FTR,
this may also show up like this.
See the second error report which makes little sense.

$ cat incomplete.cpp
class A {
public:
typedef int (A::*Foo)(int);

Foo ptr;

int foo(int value) {
return (this->*ptr)(value);
}
};

$ clang -Xclang -cxx-abi -Xclang microsoft -c incomplete.cpp
incomplete.cpp:3:19: error: incomplete type 'A' where a complete type is required
typedef int (A::Foo)(int);
^
incomplete.cpp:1:7: note: definition of 'A' is not complete until the closing '}'
class A {
^
incomplete.cpp:8:17: error: right hand operand to ->
has non pointer-to-member type 'Foo' (aka 'int')
return (this->*ptr)(value);
^ ~~~
2 errors generated.

@tritao
Copy link
Mannequin

tritao mannequin commented Sep 2, 2012

Fixed in r163078.

@nico
Copy link
Contributor Author

nico commented Sep 8, 2012

Fix got reverted.

@timurrrr
Copy link
Contributor

Ping?

@tritao
Copy link
Mannequin

tritao mannequin commented Mar 19, 2013

I still use the same fix in my local repo.

John changed the comment there to add some clarification that we need to early instantiate templates or something there. I asked for his help on how to do that on the list a couple days ago, but I could not find out how to do it with his tips.

Unless I get some example on how to do this, I really can't spend time researching on how to do it for the next weeks/months...

The fix works well for my use cases though. I think we should apply it meanwhile even if it can fail with some rare template cases... Better have something broken in rare cases than broken under all common cases.

Help me get John to accept it :-)

@timurrrr
Copy link
Contributor

Can you please share the rebased version of the patch?

[+cc: Alexander who can help with Sema/ changes]

@tritao
Copy link
Mannequin

tritao mannequin commented Mar 20, 2013

Sure, I will rebase this as soon as I can (probably later today).

@tritao
Copy link
Mannequin

tritao mannequin commented Mar 21, 2013

Member pointer patch

@rnk
Copy link
Collaborator

rnk commented Mar 21, 2013

OK, that's much smaller than r163078 was.

What's the corner case covered by the FIXME? Does it look something like this?

template
struct Foo : T {
int f;
};

struct Bar {
int b;
};

int Foo::*memptr;

Would that not instantiate Foo under Itanium?

@rnk
Copy link
Collaborator

rnk commented Mar 21, 2013

I'm also not sure this makes sense:

unsigned MicrosoftCXXABI::getMemberPointerSize(const MemberPointerType *MPT) const {
QualType Pointee = MPT->getPointeeType();
CXXRecordDecl *RD = MPT->getClass()->getAsCXXRecordDecl();
+

  • if (RD->getTypeForDecl()->isIncompleteType()) {
  • if (RD->hasAttr())
  •  return 1;
    
  • else if (RD->hasAttr())
  •  return 2;
    
  • else
  •  return 3;
    
  • }

This only seems correct for member function pointers. Data pointers seem to require one less field, which looks consistent with the code that handles complete types:

if (RD->getNumVBases() > 0) {
if (Pointee->isFunctionType())
return 3;
else
return 2;
} else if (RD->getNumBases() > 1 && Pointee->isFunctionType())
return 2;
return 1;

We should be able to query is Pointee is a function or not at this point, right? If so, we should unify the logic for complete and incomplete types.

@tritao
Copy link
Mannequin

tritao mannequin commented Mar 21, 2013

You're right, for some reason I completely forgot about data member pointers when writing this code.

Your template example makes sense, it's similar to what I was thinking, but the best thing would be to ping John.

I agree, it should be possible to unify the code to handle both cases. Can you do that?

It would be nice to add some test cases for incomplete types with the inheritance attributes too.

@rnk
Copy link
Collaborator

rnk commented Mar 21, 2013

Sounds good.

For tests I started writing some by adding -std=c++11 and using:
static_assert(sizeof(int Single::*) == sizeof(void *), "");
... Multiple, Virtual, Unspecified, cross incomplete, cross function pointer

Although, now I'm not sure that data member pointers are really only size 2 for virtual inheritance. :( I seem to have a counterexample, but I'll have to investigate further.

@tritao
Copy link
Mannequin

tritao mannequin commented Apr 17, 2013

Should be fixed as of r178283.

@tritao
Copy link
Mannequin

tritao mannequin commented Nov 26, 2021

mentioned in issue llvm/llvm-bugzilla-archive#13707

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 3, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla c++
Projects
None yet
Development

No branches or pull requests

4 participants