[clang][AST] fix ast-print of `extern <lang>` with >=2 declarators #93131

temyurchenko · 2024-05-23T04:05:46Z

Problem: the printer used to ignore all but the first declarator for unbraced language linkage declarators. Furthemore, that one would be printed without the final semicolon.

Solution: when there is more than one declarator, we print them in a braced extern <lang> block. If the original declaration was unbraced and there is one or less declarator, we omit the braces, but add the semicolon.

N.B. We are printing braces which were, in some cases, absent from the original CST. If that's an issue, I'll work on it. See the tests for the examples.

github-actions · 2024-05-23T04:06:05Z

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 · 2024-05-23T04:06:35Z

@llvm/pr-subscribers-clang

Author: Artem Yurchenko (temyurchenko)

Changes

Problem: the printer used to ignore all but the first declarator for unbraced language linkage declarators. Furthemore, that one would be printed without the final semicolon.

Solution: when there is more than one declarator, we print them in a braced extern <lang> block. If the original declaration was unbraced and there is one or less declarator, we omit the braces, but add the semicolon.

N.B. We are printing braces which were, in some cases, absent from the original CST. If that's an issue, I'll work on it. See the tests for the examples.

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

2 Files Affected:

(modified) clang/lib/AST/DeclPrinter.cpp (+4-2)
(added) clang/test/AST/ast-print-language-linkage.cpp (+27)

diff --git a/clang/lib/AST/DeclPrinter.cpp b/clang/lib/AST/DeclPrinter.cpp
index 0cf4e64f83b8d..369dfffe58667 100644
--- a/clang/lib/AST/DeclPrinter.cpp
+++ b/clang/lib/AST/DeclPrinter.cpp
@@ -1145,13 +1145,15 @@ void DeclPrinter::VisitLinkageSpecDecl(LinkageSpecDecl *D) {
     l = "C++";
   }
 
+  bool HasMoreThanOneDecl =
+      *D->decls_begin() && D->decls_begin()->getNextDeclInContext();
   Out << "extern \"" << l << "\" ";
-  if (D->hasBraces()) {
+  if (D->hasBraces() || HasMoreThanOneDecl) {
     Out << "{\n";
     VisitDeclContext(D);
     Indent() << "}";
   } else
-    Visit(*D->decls_begin());
+    VisitDeclContext(D);
 }
 
 void DeclPrinter::printTemplateParameters(const TemplateParameterList *Params,
diff --git a/clang/test/AST/ast-print-language-linkage.cpp b/clang/test/AST/ast-print-language-linkage.cpp
new file mode 100644
index 0000000000000..4998a62f280b5
--- /dev/null
+++ b/clang/test/AST/ast-print-language-linkage.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -ast-print %s -o - | FileCheck %s
+
+// CHECK: extern "C" int printf(const char *, ...);
+extern "C" int printf(const char *...);
+
+// CHECK: extern "C++" {
+// CHECK-NEXT:   int f(int);
+// CHECK-NEXT:   int g(int);
+// CHECK-NEXT: }
+extern "C++" int f(int), g(int);
+
+// CHECK: extern "C" {
+// CHECK-NEXT:  void foo();
+// CHECK-NEXT:  int x;
+// CHECK-NEXT:  int y;
+// CHECK-NEXT: }
+extern "C" {
+  void foo(void);
+  int x, y;
+}
+
+// CHECK: extern "C" {
+// CHECK-NEXT: }
+extern "C" {}
+
+// CHECK: extern "C++" ;
+extern "C++";

temyurchenko · 2024-05-23T04:07:05Z

Fixes #93085

clang/test/AST/ast-print-language-linkage.cpp

clang/lib/AST/DeclPrinter.cpp

clang/test/AST/ast-print-language-linkage.cpp

AaronBallman

I think we're going to be printing incorrect code either way, but with this patch, we're more likely to be closer to the original intent of the code than without the patch. So even though this isn't perfect, it's forward progress and I think we're fine to land it as-is. However, if we can fix it correctly, that would be preferred.

clang/test/AST/ast-print-language-linkage.cpp

github-actions · 2024-05-29T00:38:24Z

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

erichkeane

I'm in favor, let aaron take a look, else LGTM!

clang/test/AST/ast-print-language-linkage.cpp

AaronBallman

LGTM aside from some coding style nits. Because AST printing is not part of the public interface for Clang, I don't think these changes need a release note.

clang/lib/AST/DeclPrinter.cpp

Problem: the printer used to ignore all but the first declarator for unbraced language linkage declarators. Furthemore, that one would be printed without the final semicolon. Solution: for unbraced case we traverse all declarators via `VisitDeclContext`. Furthermore, in appropriate visitors we query for whether they are a part of the unbraced extern language linkage spec, and if so, print appropriately.

temyurchenko · 2024-05-30T17:50:15Z

What should be the next step following the approvals?

erichkeane · 2024-05-30T17:53:27Z

What should be the next step following the approvals?

If you do not have 'squash and merge' rights here, one of us can do it for you once testing completes.

temyurchenko · 2024-05-30T17:54:35Z

What should be the next step following the approvals?

If you do not have 'squash and merge' rights here, one of us can do it for you once testing completes.

I don't have them, would be glad if you could do that!

github-actions · 2024-05-30T20:19:04Z

@temyurchenko 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 receive 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!

gulfemsavrun · 2024-05-30T23:16:17Z

We started seeing an lldb test failure, and I bisected to this commit:

--
Command Output (stderr):
--
FAIL: LLDB (/b/s/w/ir/x/w/llvm_build/bin/clang-x86_64) :: test_completions (TestDAP_completions.TestDAP_completions.test_completions)
FAIL: LLDB (/b/s/w/ir/x/w/llvm_build/bin/clang-x86_64) :: test_completions (TestDAP_completions.TestDAP_completions.test_completions)
======================================================================
ERROR: test_completions (TestDAP_completions.TestDAP_completions.test_completions)
   Tests the completion request at different breakpoints
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/b/s/w/ir/x/w/llvm-llvm-project/lldb/packages/Python/lldbsuite/test/decorators.py", line 150, in wrapper
    return func(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^
  File "/b/s/w/ir/x/w/llvm-llvm-project/lldb/test/API/tools/lldb-dap/completions/TestDAP_completions.py", line 89, in test_completions
    self.dap_server.get_completions("`log enable  "),
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/b/s/w/ir/x/w/llvm-llvm-project/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py", line 448, in get_completions
    response = self.request_completions(text, frameId)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/b/s/w/ir/x/w/llvm-llvm-project/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py", line 979, in request_completions
    return self.send_recv(command_dict)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/b/s/w/ir/x/w/llvm-llvm-project/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py", line 324, in send_recv
    raise ValueError(desc)
ValueError: no response for "completions"
Config=x86_64-/b/s/w/ir/x/w/llvm_build/bin/clang
======================================================================
ERROR: test_completions (TestDAP_completions.TestDAP_completions.test_completions)
   Tests the completion request at different breakpoints
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/b/s/w/ir/x/w/llvm-llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 2017, in tearDown
    Base.tearDown(self)
  File "/b/s/w/ir/x/w/llvm-llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 1103, in tearDown
    hook()  # try the plain call and hope it works
    ^^^^^^
  File "/b/s/w/ir/x/w/llvm-llvm-project/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py", line 375, in cleanup
    self.dap_server.request_disconnect(terminateDebuggee=True)
  File "/b/s/w/ir/x/w/llvm-llvm-project/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py", line 666, in request_disconnect
    return self.send_recv(command_dict)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/b/s/w/ir/x/w/llvm-llvm-project/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py", line 318, in send_recv
    self.send_packet(command)
  File "/b/s/w/ir/x/w/llvm-llvm-project/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py", line 273, in send_packet
    self.send.flush()
BrokenPipeError: [Errno 32] Broken pipe
Config=x86_64-/b/s/w/ir/x/w/llvm_build/bin/clang
----------------------------------------------------------------------
Ran 1 test in 1.600s

FAILED (errors=2)

--

https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket/8746482644341901905/+/u/lldb/test/stdout

temyurchenko · 2024-05-31T00:28:45Z

We started seeing an lldb test failure, and I bisected to this commit:

I see the source of the issue, I'm thinking how to address it best. If it's urgent, I can produce an urgent fix.

gulfemsavrun · 2024-05-31T01:34:46Z

We started seeing an lldb test failure, and I bisected to this commit:

I see the source of the issue, I'm thinking how to address it best. If it's urgent, I can produce an urgent fix.

Can you please revert it in the meantime because it is breaking our builders? Instead of a quick fix, I recommend a revert and a reland with a fix. I can help you to verify whether it actually fixes the issue in our builders.

temyurchenko · 2024-05-31T01:44:34Z

We started seeing an lldb test failure, and I bisected to this commit:

I see the source of the issue, I'm thinking how to address it best. If it's urgent, I can produce an urgent fix.

Can you please revert it in the meantime because it is breaking our builders? Instead of a quick fix, I recommend a revert and a reland with a fix. I can help you to verify whether it actually fixes the issue in our builders.

Unfortunately, I don't have the right to revert.

…ators" (#93912) Reverts #93131 because it broke some lldb tests on the Fuchsia Clang toolchain builders. https://ci.chromium.org/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8746482644341901905/infra

gulfemsavrun · 2024-05-31T01:53:34Z

Unfortunately, I don't have the right to revert.

No worries, and I reverted it for you. When you have the fix ready, I'm happy to verify it on our builders before you reland.

temyurchenko · 2024-05-31T02:14:27Z

No worries, and I reverted it for you. When you have the fix ready, I'm happy to verify it on our builders before you reland.

Can you also reopen this PR, please? For documentation/history reasons, it's probably better to provide a fix within the scope of this PR.

erichkeane · 2024-05-31T13:30:50Z

No worries, and I reverted it for you. When you have the fix ready, I'm happy to verify it on our builders before you reland.

Can you also reopen this PR, please? For documentation/history reasons, it's probably better to provide a fix within the scope of this PR.

I don't think our github is configured to allow that. You'll have to start a new PR.

…lvm#93131) Problem: the printer used to ignore all but the first declarator for unbraced language linkage declarators. Furthemore, that one would be printed without the final semicolon. Solution: when there is more than one declarator, we print them in a braced `extern <lang>` block. If the original declaration was unbraced and there is one or less declarator, we omit the braces, but add the semicolon. **N.B.** We are printing braces which were, in some cases, absent from the original CST. If that's an issue, I'll work on it. See the tests for the examples.

…lvm#93131) Problem: the printer used to ignore all but the first declarator for unbraced language linkage declarators. Furthemore, that one would be printed without the final semicolon. Solution: for unbraced case we traverse all declarators via `VisitDeclContext`. Furthermore, in appropriate visitors we query for whether they are a part of the unbraced extern language linkage spec, and if so, print appropriately.

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels May 23, 2024

zyn0217 linked an issue May 23, 2024 that may be closed by this pull request

-ast-print of an extern-C declaration skips closing semi-colon #93085

Closed

erichkeane self-requested a review May 23, 2024 16:02

erichkeane reviewed May 23, 2024

View reviewed changes

clang/test/AST/ast-print-language-linkage.cpp Outdated Show resolved Hide resolved

clang/lib/AST/DeclPrinter.cpp Outdated Show resolved Hide resolved

clang/test/AST/ast-print-language-linkage.cpp Show resolved Hide resolved

AaronBallman reviewed May 28, 2024

View reviewed changes

clang/test/AST/ast-print-language-linkage.cpp Outdated Show resolved Hide resolved

temyurchenko force-pushed the main branch 2 times, most recently from 3e19f75 to ae5225a Compare May 29, 2024 00:35

temyurchenko force-pushed the main branch 3 times, most recently from 0fd91ab to a8f1503 Compare May 29, 2024 00:52

erichkeane approved these changes May 29, 2024

View reviewed changes

clang/test/AST/ast-print-language-linkage.cpp Show resolved Hide resolved

AaronBallman approved these changes May 30, 2024

View reviewed changes

clang/lib/AST/DeclPrinter.cpp Outdated Show resolved Hide resolved

clang/lib/AST/DeclPrinter.cpp Outdated Show resolved Hide resolved

clang/lib/AST/DeclPrinter.cpp Outdated Show resolved Hide resolved

temyurchenko force-pushed the main branch from a8f1503 to b1490a3 Compare May 30, 2024 17:16

temyurchenko force-pushed the main branch from b1490a3 to fac4547 Compare May 30, 2024 17:27

erichkeane merged commit 7b80489 into llvm:main May 30, 2024
7 checks passed

gulfemsavrun mentioned this pull request May 31, 2024

Revert "[clang][AST] fix ast-print of extern <lang> with >=2 declarators" #93912

Merged

temyurchenko mentioned this pull request Jun 7, 2024

[clang][AST] fix ast-print of extern <lang> with >=2 declarators, fixed #93913

Merged

[clang][AST] fix ast-print of extern <lang> with >=2 declarators #93131

[clang][AST] fix ast-print of extern <lang> with >=2 declarators #93131

Uh oh!

Conversation

temyurchenko commented May 23, 2024

Uh oh!

github-actions bot commented May 23, 2024

Uh oh!

llvmbot commented May 23, 2024

Uh oh!

temyurchenko commented May 23, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented May 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

temyurchenko commented May 30, 2024

Uh oh!

erichkeane commented May 30, 2024

Uh oh!

temyurchenko commented May 30, 2024

Uh oh!

Uh oh!

github-actions bot commented May 30, 2024

Uh oh!

gulfemsavrun commented May 30, 2024

Uh oh!

temyurchenko commented May 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gulfemsavrun commented May 31, 2024

Uh oh!

temyurchenko commented May 31, 2024

Uh oh!

gulfemsavrun commented May 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

temyurchenko commented May 31, 2024

Uh oh!

erichkeane commented May 31, 2024

Uh oh!

Uh oh!

[clang][AST] fix ast-print of `extern <lang>` with >=2 declarators #93131

[clang][AST] fix ast-print of `extern <lang>` with >=2 declarators #93131

github-actions bot commented May 29, 2024 •

edited

Loading

temyurchenko commented May 31, 2024 •

edited

Loading

gulfemsavrun commented May 31, 2024 •

edited

Loading