Fix a bug which access nullptr and cause segmentation fault

[yamaguchi]

Summary

Fixed a bug [1].
ExistingInit was a null pointer, so I added a check.
After this change, I could get similar result as GCC [2].

[1] http://bugs.llvm.org/show_bug.cgi?id=32280
[2] https://pastebin.com/mNFKDubm

[teemperor]

Please fix these things:

• Formatting: Indentation of the if body. Missing space before and after the parantheses of if. We have a tool called clang-format that automatically applies our code-style to patches, so please try that out.

• NULL is C-style code. Please use a nullptr instead. Or better just use if (ExistingInit) {.

• If you run this on the test-case, it would print half a diagnostic (e.g. the warning) and then stop in the mid:

~/llvm/build/bin/clang -cc1  test.c
test.c:41:6: error: field designator 'dog' does not refer to any field in type 'struct_0'
    .dog = 0x00000000,
     ^
warning: initializer overrides prior initialization of this subobject

The warning: is obviously missing the note: Previous initialization here and doesn't make a lot of sense that way, so let's skip the whole thing altogether. We should check if ExistingInit is a nullptr before we start printing the warning (which is a few lines above the note printing).

• Please attach the example from the bug-report as a test-case (with the proper // RUN line so people can actually run it). See the other tests for examples).

• When uploading your diff, please make sure your diff has enough context, otherwise everyone sees this Context not available. warning in the code review. Just do something like -U999 when creating your diff for an review as described here.

[yamaguchi]

Moved if (ExistingInit) above, so that warning will not printed without notes, and fixed indents.

[yamaguchi]

Add a testcase.

[yamaguchi]

Moved comment inside if (ExistingInit).

[yamaguchi]

Made unified diff for the testcase and SemaInit.cpp.

[yamaguchi]

Add relative path to the file.

[ahatanak]

clang/test/Sema/sema-segvcheck.c

3 ↗

(On Diff #94090)

You can simplify the test case. Compiling the following code still segfaults:

typedef struct {
  unsigned long long house;
} struct_0;


typedef union {
  unsigned cows;
  unsigned char c;
} union_1;


typedef struct {
  struct_0 s0;
  union_1 s1;
} struct_2;

struct_2 s = {
  .s0 = {
    .dog = 0x00000009,
  },

  .s1 = {
    .cows = 0x00000055,
    .c = 1,
  },
};

Also, would it be better to add this test to an existing file (e.g., test/Sema/designated-initializers.c) rather than creating a new file?

[ruiu]

Is this a minimal test case that can produce the issue? It'd be awesome if you can reduce it.

sema-segvcheck.c is not a good name for this test because that name can be used for any crash bug. You want to see other files in the same directory to name your file so that it's consistent with other files. If you don't come up with a name, I'd name it pr32280.c.

[v.g.vassilev]

@yamaguchi, I'd support Akira's comment. Could you place the minimal test example in the suggested file?

[v.g.vassilev]

ping...

[yamaguchi]

I've been trying to minimal the testcase, add comments to describe what it is testing, and fix styles of the testcase properly.
However, I don't have clear idea what will be the best. I would like to ask for the advice.

[v.g.vassilev]

LGTM.

[yamaguchi]

I don't have a commit access. Can you commit?

[v.g.vassilev]

Landed in r300313.

SemaInit.cpp

     if (RT->getDecl()->isUnion()) {
       FieldIndex = 0;
       if (!VerifyOnly) {
         FieldDecl *CurrentField = StructuredList->getInitializedFieldInUnion();
         if (CurrentField && !declaresSameEntity(CurrentField, *Field)) {
           assert(StructuredList->getNumInits() == 1
                  && "A union should never have more than one initializer!");
 
-          // We're about to throw away an initializer, emit warning.
-          SemaRef.Diag(D->getFieldLoc(),
-                       diag::warn_initializer_overrides)
-            << D->getSourceRange();
           Expr *ExistingInit = StructuredList->getInit(0);
-          SemaRef.Diag(ExistingInit->getLocStart(),
-                       diag::note_previous_initializer)
-            << /*FIXME:has side effects=*/0
-            << ExistingInit->getSourceRange();
+          if (ExistingInit) {
+            // We're about to throw away an initializer, emit warning.
+            SemaRef.Diag(D->getFieldLoc(),
+                         diag::warn_initializer_overrides)
+              << D->getSourceRange();
+            SemaRef.Diag(ExistingInit->getLocStart(),
+                         diag::note_previous_initializer)
+              << /*FIXME:has side effects=*/0
+              << ExistingInit->getSourceRange();
+          }
 
           // remove existing initializer
           StructuredList->resizeInits(SemaRef.Context, 0);
           StructuredList->setInitializedFieldInUnion(nullptr);
         }
 
         StructuredList->setInitializedFieldInUnion(*Field);
       }