[PATCH] D82740: [libclang]: check validity before visiting Stmt node

[Milian Wolff via Phabricator via cfe-commits]

milianw abandoned this revision.
milianw added a comment.

great, thanks for the help - please land the other change then


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82740/new/

https://reviews.llvm.org/D82740



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82740: [libclang]: check validity before visiting Stmt node

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

@milianw I just approved https://reviews.llvm.org/D82629 - I feel like that 
patch is addressing the core issue.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82740/new/

https://reviews.llvm.org/D82740



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82740: [libclang]: check validity before visiting Stmt node

[Milian Wolff via Phabricator via cfe-commits]

milianw added a comment.

@jkorous ping? can you chime in on either of the two patches? I'm fine with 
either, and D82629  comes with a test too. So 
maybe let's just go with that one? If so, could you integrate that one please?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82740/new/

https://reviews.llvm.org/D82740



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82740: [libclang]: check validity before visiting Stmt node

[Milian Wolff via Phabricator via cfe-commits]

milianw added a comment.

@jkorous how would you use a debugger (would be GDB for me) to find the source 
- I would have to use RR or something like that to see why and where the 
invalid node is added, no?

I also don't have the breaking code at hand anymore, I can try to come up with 
a way to reproduce it, but as I said it was very non-intuitive and in a large 
file. I think it was related to parse errors in a statement for a variable that 
was then captured in a lambda - but that's just a hunch I got from the 
backtrace so far.

Generally, it's not very straight forward to build a reduced crash test for 
clang-c. Maybe one could use c-reduce with a minimal visitor... And preprocess 
the file to have it standalone. I'll see if I get a chance to look into this 
any time soon, I doubt it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82740/new/

https://reviews.llvm.org/D82740



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82740: [libclang]: check validity before visiting Stmt node

[Milian Wolff via Phabricator via cfe-commits]

milianw added a comment.

Also see https://reviews.llvm.org/D82629 which I believe is also a fix for this 
crash. But I didn't use a VLA in my code, but maybe the parse error just 
triggered something similar like that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82740/new/

https://reviews.llvm.org/D82740



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82740: [libclang]: check validity before visiting Stmt node

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

Your patch definitely fixes the symptoms of the bug. I just want to make sure 
that we aren't covering some logic error with a bandaid as it would be harder 
to find out the root cause once we land this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82740/new/

https://reviews.llvm.org/D82740



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82740: [libclang]: check validity before visiting Stmt node

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

@milianw could you try to reduce the reproducer you have? Maybe take lldb and 
see where does the `nullptr` come from?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82740/new/

https://reviews.llvm.org/D82740



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82740: [libclang]: check validity before visiting Stmt node

[Milian Wolff via Phabricator via cfe-commits]

milianw added a comment.

I'm not sure how to write a unit test for this, but I ran into a reproducible 
crash with a complex C++ file which got fixed by this patch


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82740/new/

https://reviews.llvm.org/D82740



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82740: [libclang]: check validity before visiting Stmt node

[Milian Wolff via Phabricator via cfe-commits]

milianw created this revision.
milianw added reviewers: bkramer, yvvan, nik.
Herald added subscribers: cfe-commits, arphaman.
Herald added a project: clang.

Fixes crash when visiting a partial AST after encountering
a parse error in the input code:

  #5  clang::Stmt::getStmtClass (this=) at 
llvm-project/clang/include/clang/AST/Stmt.h:1125
  #6  clang::cxcursor::MakeCXCursor (S=S@entry=0x0, Parent=0x0, 
TU=0x7f8c6c5186b0, RegionOfInterest=...) at 
llvm-project/clang/tools/libclang/CXCursor.cpp:132
  #7  0x7f8caddc96e5 in clang::cxcursor::CursorVisitor::EnqueueWorkList 
(this=this@entry=0x7f8c9bffc330, WL=..., S=S@entry=0x0) at 
llvm-project/clang/tools/libclang/CIndex.cpp:3028
  #8  0x7f8cadde4e2c in clang::cxcursor::CursorVisitor::Visit 
(this=this@entry=0x7f8c9bffc330, S=0x0) at 
llvm-project/clang/tools/libclang/CIndex.cpp:3256
  #9  0x7f8cadde4604 in clang::cxcursor::CursorVisitor::RunVisitorWorkList 
(this=this@entry=0x7f8c9bffc330, WL=...) at 
llvm-project/clang/tools/libclang/CIndex.cpp:3214
  #10 0x7f8cadde4e37 in clang::cxcursor::CursorVisitor::Visit 
(this=this@entry=0x7f8c9bffc330, S=0x7f8c6c17dc30) at 
llvm-project/clang/tools/libclang/CIndex.cpp:3257
  #11 0x7f8cadddeccb in clang::cxcursor::CursorVisitor::VisitChildren 
(this=this@entry=0x7f8c9bffc330, Cursor=...) at 
llvm-project/clang/tools/libclang/CIndex.cpp:515
  #12 0x7f8cadde688f in clang_visitChildren (parent=..., 
visitor=0x7f8caef6ddda <(anonymous namespace)::visitCursor(CXCursor, CXCursor, 
CXClientData)>, client_data=0x7f8c9bffe3b0) at 
llvm-project/clang/tools/libclang/CIndex.cpp:4450


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D82740

Files:
  clang/tools/libclang/CIndex.cpp


Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -3243,6 +3243,8 @@
 }
 
 bool CursorVisitor::Visit(const Stmt *S) {
+  if (!S)
+return false;
   VisitorWorkList *WL = nullptr;
   if (!WorkListFreeList.empty()) {
 WL = WorkListFreeList.back();


Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -3243,6 +3243,8 @@
 }
 
 bool CursorVisitor::Visit(const Stmt *S) {
+  if (!S)
+return false;
   VisitorWorkList *WL = nullptr;
   if (!WorkListFreeList.empty()) {
 WL = WorkListFreeList.back();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits