relocation diagnostics asserts in response to code that is broken for unrelated reasons #awesome

[ojhunt]

I made an error in one of my test cases and got a crash due to a null value. After much confusion I realized the problem is actually that I had redefined the same struct multiple times, and that's what caused the unexpected state.

The test case:

struct Foo  {
  Foo(const Foo&);
  ~Foo();
};

struct Foo {
  Foo();
  int;
};

struct Wrapper {
  union {
    Foo p;
  } u;
};
static_assert(__builtin_is_cpp_trivially_relocatable(Wrapper));

The assertion is triggered under DiagnoseNonTriviallyRelocatableReason in this code:

  if (!D->hasSimpleMoveConstructor() && !D->hasSimpleCopyConstructor()) {
    const auto *Decl = cast<CXXConstructorDecl>(
        LookupSpecialMemberFromXValue(SemaRef, D, /*Assign=*/false));
    if (Decl && Decl->isUserProvided())
      SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
          << diag::TraitNotSatisfiedReason::UserProvidedCtr
          << Decl->isMoveConstructor() << Decl->getSourceRange();
  }

The issue is the cast< CXXConstructorDecl > as LookupSpecialMemberFromXValue can return null. The cast<> is followed by a null check so it's presumably expected that LookupSpecialMemberFromXValue returns null, in which case the fix is likely just to replace cast with cast_or_null. If it's expected that the cast itself may fail then I assume there's dyn_cast_or_null that should be used.

[llvmbot]

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

1. Check that no other contributor is working on this issue. If someone is assigned to the issue or claimed to be working on it, ping the person. After one week without a response, the assignee may be changed.
2. Leave a comment indicating that you are working on the issue, or just create a pull request after following the steps below. Mention this issue in the description of the pull request.
3. Fix the issue locally.
4. Run the test suite locally. Remember that the subdirectories under test/ create fine-grained testing targets, so you can e.g. use make check-clang-ast to only run Clang's AST tests.
5. Create a Git commit.
6. Run git clang-format HEAD~1 to format your changes.
7. Open a pull request to the upstream repository on GitHub. Detailed instructions can be found in GitHub's documentation. Mention this issue in the description of the pull request.

If you have any further questions about this issue, don't hesitate to ask via a comment in the thread below.

[llvmbot]

@llvm/issue-subscribers-good-first-issue

Author: Oliver Hunt (ojhunt)

I made an error in one of my test cases and got a crash due to a null value. After much confusion I realized the problem is actually that I had redefined the same struct multiple times, and that's what caused the unexpected state.

The test case:

struct Foo  {
  Foo(const Foo&);
  ~Foo();
};

struct Foo {
  Foo();
  int;
};

struct Wrapper {
  union {
    Foo p;
  } u;
};
static_assert(__builtin_is_cpp_trivially_relocatable(Wrapper));

The assertion is triggered under DiagnoseNonTriviallyRelocatableReason in this code:

  if (!D->hasSimpleMoveConstructor() && !D->hasSimpleCopyConstructor()) {
    const auto *Decl = cast<CXXConstructorDecl>(
        LookupSpecialMemberFromXValue(SemaRef, D, /*Assign=*/false));
    if (Decl && Decl->isUserProvided())
      SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
          << diag::TraitNotSatisfiedReason::UserProvidedCtr
          << Decl->isMoveConstructor() << Decl->getSourceRange();
  }

The issue is the cast< CXXConstructorDecl > as LookupSpecialMemberFromXValue can return null. The cast<> is followed by a null check so it's presumably expected that LookupSpecialMemberFromXValue returns null, in which case the fix is likely just to replace cast with cast_or_null. If it's expected that the cast itself may fail then I assume there's dyn_cast_or_null that should be used.

[ojhunt]

pre-assigning to @cor3ntin because I blame him for everything, but this is a trivial fix that any one can do if they're wanting to learn how to setup a checkout and build configuration, learn how to add and run tests, etc

[llvmbot]

@llvm/issue-subscribers-clang-frontend

Author: Oliver Hunt (ojhunt)

I made an error in one of my test cases and got a crash due to a null value. After much confusion I realized the problem is actually that I had redefined the same struct multiple times, and that's what caused the unexpected state.

The test case:

struct Foo  {
  Foo(const Foo&);
  ~Foo();
};

struct Foo {
  Foo();
  int;
};

struct Wrapper {
  union {
    Foo p;
  } u;
};
static_assert(__builtin_is_cpp_trivially_relocatable(Wrapper));

The assertion is triggered under DiagnoseNonTriviallyRelocatableReason in this code:

  if (!D->hasSimpleMoveConstructor() && !D->hasSimpleCopyConstructor()) {
    const auto *Decl = cast<CXXConstructorDecl>(
        LookupSpecialMemberFromXValue(SemaRef, D, /*Assign=*/false));
    if (Decl && Decl->isUserProvided())
      SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
          << diag::TraitNotSatisfiedReason::UserProvidedCtr
          << Decl->isMoveConstructor() << Decl->getSourceRange();
  }

The issue is the cast< CXXConstructorDecl > as LookupSpecialMemberFromXValue can return null. The cast<> is followed by a null check so it's presumably expected that LookupSpecialMemberFromXValue returns null, in which case the fix is likely just to replace cast with cast_or_null. If it's expected that the cast itself may fail then I assume there's dyn_cast_or_null that should be used.

[ojhunt]

I was looking at my original test case and the duplicate definition failure did not exist, with a bunch of trimming:

struct Polymorphic  {
  virtual ~Polymorphic();
};

struct UnionOfPolymorphic {
  union {
    Polymorphic p;
    int i;
  } u;
};

static_assert(__builtin_is_cpp_trivially_relocatable(UnionOfPolymorphic));

This triggers the assertion even though there's no error in the code. Note that the static_assert is not necessarily reflecting the actual expected result, but is what is necessary to trigger the diagnostic path.

[cor3ntin]

Yeah, I'll look at that today