Fix a bug which access nullptr and cause segmentation fault
ClosedPublic

Authored by yamaguchi on Apr 2 2017, 9:56 PM.

Details

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

Diff Detail

yamaguchi created this revision.Apr 2 2017, 9:56 PM
yamaguchi added reviewers: ruiu, v.g.vassilev, teemperor.
yamaguchi removed subscribers: ruiu, v.g.vassilev, teemperor.
teemperor requested changes to this revision.Apr 2 2017, 10:51 PM

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.
This revision now requires changes to proceed.Apr 2 2017, 10:51 PM
yamaguchi updated this revision to Diff 93857.Apr 3 2017, 7:32 AM

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

yamaguchi updated this revision to Diff 93858.Apr 3 2017, 7:40 AM

Add a testcase.

yamaguchi updated this revision to Diff 93860.Apr 3 2017, 7:51 AM

Moved comment inside if (ExistingInit).

yamaguchi updated this revision to Diff 93864.Apr 3 2017, 7:56 AM

Made unified diff for the testcase and SemaInit.cpp.

yamaguchi updated this revision to Diff 94090.Apr 4 2017, 10:00 AM

Add relative path to the file.

ahatanak added inline comments.
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 added a comment.Apr 4 2017, 10:32 AM

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.

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

yamaguchi updated this revision to Diff 95064.Apr 12 2017, 5:46 PM

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.

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

Landed in r300313.

teemperor accepted this revision.Apr 19 2017, 2:59 AM
This revision is now accepted and ready to land.Apr 19 2017, 2:59 AM
teemperor closed this revision.Apr 19 2017, 2:59 AM