[CLANGD] Do not crash on designator initialization of union

[alirezamoshtaghi]

No description provided.

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-clang-tools-extra
@llvm/pr-subscribers-clangd

@llvm/pr-subscribers-clang

Author: None (alirezamoshtaghi)

Changes

---

Full diff: https://github.com/llvm/llvm-project/pull/83369.diff

2 Files Affected:

• (added) clang-tools-extra/clangd/test/designator_init.test (+31)
• (modified) clang/lib/AST/Expr.cpp (+12-2)

diff --git a/clang-tools-extra/clangd/test/designator_init.test b/clang-tools-extra/clangd/test/designator_init.test
new file mode 100644
index 00000000000000..739f2bfab54bcf
--- /dev/null
+++ b/clang-tools-extra/clangd/test/designator_init.test
@@ -0,0 +1,31 @@
+//# RUN: rm -rf %t.dir/* && mkdir -p %t.dir
+//# RUN: echo '[{"directory": "%/t.dir", "command": "clang -x c -c %s", "file": "%s"}]' > %t.dir/compile_commands.json
+//# RUN: clangd --compile-commands-dir=%t.dir --check=%s 2>&1 | FileCheck %s
+
+typedef struct S {
+  unsigned char id;
+  union {
+    unsigned int mask;
+    struct {
+      unsigned int unused:10;
+      unsigned int reserved:3;
+      unsigned int rest:19;
+    };
+  };
+} __attribute__((packed)) S_t;
+
+typedef struct H {
+  unsigned char hid;
+  unsigned int val;
+} handler_t;
+
+struct S
+get_foo (handler_t *h)
+{
+  S_t retval =
+    {.id=h->hid,
+     .mask=h->val};
+  return retval;
+}
+
+// CHECK: All checks completed, 0 errors
diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index b4de2155adcebd..33eeeda89fe7a5 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -4601,11 +4601,21 @@ SourceRange DesignatedInitExpr::getDesignatorsSourceRange() const {
 SourceLocation DesignatedInitExpr::getBeginLoc() const {
   auto *DIE = const_cast<DesignatedInitExpr *>(this);
   Designator &First = *DIE->getDesignator(0);
-  if (First.isFieldDesignator())
-    return GNUSyntax ? First.getFieldLoc() : First.getDotLoc();
+  if (First.isFieldDesignator()) {
+    /* search all designators in case the first one is not
+       initialized */
+    for (unsigned int i=0; i<DIE->size(); i++) {
+      Designator &Des = *DIE->getDesignator(i);
+      SourceLocation retval = GNUSyntax ? Des.getFieldLoc() : Des.getDotLoc();
+      if (!retval.isValid ())
+	continue;
+      return retval;
+    }
+  }
   return First.getLBracketLoc();
 }
 
+
 SourceLocation DesignatedInitExpr::getEndLoc() const {
   return getInit()->getEndLoc();
 }

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[HighCommander4]

Thanks for the patch!

I wrote some notes in the issue (#83185) to help me understand the problem the patch is fixing.

The fix looks good to me.

Regarding the test:

• Usually, for fixes in frontend code (clang directory), it's preferred to have a test which is also in clang. For an issue like this (an AST node having a bad source location), a common way to test it is a lit test with runs ast-dump on the source file and checks the node's source location in the AST dump.
  • However, DesignatedInitExpr does not appear at all in the AST dump. (The reason is that it's only part of the syntactic form of InitListExpr, and the AST dumping code only prints the semantic form.)
• Therefore, I would say writing a clangd test is fine.
  • For clangd tests, we generally prefer unit tests. Could you put the testcase in ExtractVariableTests.cpp, like this? The input is the source code with the selection that causes the crash annotated with [[]], and then you can call EXPECT_AVAILABLE() on it (with the fix, the refactoring is in fact available for this selection).

Thanks!

[HighCommander4]

This test case does not fail without the fix.

The reason is that the selection [[x]] does not trigger the crash.

The selection needs to be on the [[->]] to trigger the crash (and then, the result will be AVAILABLE).

Otherwise looks good, thanks!

[HighCommander4]

LGTM

[github-actions]

@alirezamoshtaghi Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested
by our build bots. If there is a problem with a build, you may recieve a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as
the builds can include changes from many authors. It is not uncommon for your
change to be included in a build that fails due to someone else's changes, or
infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself.
This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!