@llvm/issue-subscribers-clang-query

Author: Vlad Serebrennikov (Endilll)

`clang-query` crashes with the following backtrace:

https://gcc.godbolt.org/z/P69MjKeGs
</details>

@llvm/issue-subscribers-c-1

Author: Vlad Serebrennikov (Endilll)

`clang-query` crashes with the following backtrace:

https://gcc.godbolt.org/z/P69MjKeGs
</details>

This feature appears to be quite fragile as it crashes quite readily: https://godbolt.org/z/xj5xWP5x6, https://godbolt.org/z/c63dchdad, https://godbolt.org/z/ccz5crWW6

This has also started to cause some maintenance burden within Clang itself (I had a developer trying to add a templated base class to the AST hierarchy and could not because it would fail to build the .inc file used by generate_cxx_src_locs.py).

There's not been any maintenance done on this feature for three years and it doesn't seem to be particularly usable in practice, so I am considering pulling this functionality out of clang-query to reduce the burden on the community. However, before doing that, I'd like to hear from @steveire (as the original author for this functionality) about whether they are planning to address these kinds of issues. If the functionality is going to be actively maintained, it's still useful. However, if this is unmaintained, I think I will remove it sometime in the Clang 19 release cycle.

I don't agree that the feature is not useful. Just today I helped someone to understand what source locations he needed using this feature. This bug report also shows it is being used.

Deleting the feature due to some crashes with repros seems extreme. Why not find a way to avoid the crashes instead?

This bug report also shows it is being used.

I was just playing around with clang-query, figuring out which information I can get out of it.

Deleting the feature due to some crashes with repros seems extreme. Why not find a way to avoid the crashes instead?

It would make total sense if there was a clang-query maintainer with some spare cycles. Unfortunately, this doesn't seem the case. Even though I'm the author of the bug, I think we shouldn't keep around code we don't maintain.

Well, really two people are needed, ideally both with about the same contributions to clang-query, so that they can review each others code. I can be one of them, but not both :).

But your point applies to all of clang-query generally. If it doesn't have a maintainer, then delete it?

I'm still the lead maintainer for clang-query (so I can review any fixes that are provided for this) and the reason I'm suggesting we delete this functionality is because it's got quite a bit of complexity (which I pointed out as a concern when I originally reviewed adding this because it's using multiple levels of code generation) and that complexity is now getting in the way of maintaining Clang. The functionality is almost entirely unusable in its current state (crashes on very, very basic queries like recordDecl() and extremely simple code like struct S{};), so that's why I was asking if you were planning to maintain it as the original author. If you're not able to do so, then I can pull it out of clang-query and it can be resurrected once someone has the bandwidth to do so. But because it's causing problems for work happening in Clang, there's extra pressure to do something. If this feature mostly worked or was actively maintained, then I can tell the other developers "sorry, we're not pulling a feature from clang-query to make your life easier" but there's no real reason to do so when the feature isn't particularly functional and nobody is willing to put in the time to make it functional.

It's worth noting that this appears to have been crashing since Clang 13 (2021) and this is the only report of an issue, so it's not obvious that there's significant use of it in the wild (and there's not a particularly easy way for us to find out if that's correct or not).

Without actually investigating the cause of the crashes, it's premature to declare the feature dead :). Obviously the feature does work for demo-able cases https://godbolt.org/z/M7Y8sK9oo . These are just bugs.

If I make the crashes not happen (one way or another - possibly by excluding "hard" cases), will you be deleting the feature anyway?

If someone is willing to put effort into maintaining the feature then I think it can stay in clang-query because it's useful and no longer a maintenance burden.

If the feature has been crashing on such trivial code for multiple years with only one bug report from happenstance use, that suggests there's not wide use of the feature in the wild. If there's not much use of the feature in the wild and we're running into problems from it outside of clang-query, that makes it a maintenance burden for us to keep around. However, if someone is willing to actively maintain the feature (keep fixing crashes until the feature is stable enough for significant use, ensure test coverage is sufficient so that we can hopefully avoid regressing the feature accidentally or be proactively watching for bug reports), that eliminates the maintenance burden for the community and demonstrates there is interest in supporting the feature even if it doesn't have wide adoption yet. (So it's not "this has bugs, let's rip it out", it's "this appears to have been very broken for multiple years, isn't well-tested, is perhaps not used all that often, and now is causing pain elsewhere in the project, so is it needed/maintained?")

I think my llvm-project access was revoked, but if you want to fix the crashes you can apply this patch:

Author: Stephen Kelly <steveire@gmail.com>
Date:   Wed Apr 24 23:27:31 2024 +0100

    Avoid calling AST APIs which crash

diff --git a/clang/lib/Tooling/DumpTool/ASTSrcLocProcessor.cpp b/clang/lib/Tooling/DumpTool/ASTSrcLocProcessor.cpp
index 42691d556d98..3e0c7cb87788 100644
--- a/clang/lib/Tooling/DumpTool/ASTSrcLocProcessor.cpp
+++ b/clang/lib/Tooling/DumpTool/ASTSrcLocProcessor.cpp
@@ -195,6 +195,21 @@ CaptureMethods(std::string TypeString, const clang::CXXRecordDecl *ASTClass,
   return Methods;
 }
 
+void FilterUnsafeAPIs(StringRef ClassName, ClassData& CD)
+{
+  // Some APIs in Clang AST can not be called naively, so filter them out
+  // of the API result for those AST node types
+
+  if (ClassName == "CXXRecordDecl")
+  {
+    llvm::erase(CD.TypeSourceInfos, "getLambdaTypeInfo");
+  }
+  if (ClassName == "AutoTypeLoc")
+  {
+    llvm::erase(CD.DeclNameInfos, "getConceptNameInfo");
+  }
+}
+
 void ASTSrcLocProcessor::run(const MatchFinder::MatchResult &Result) {
 
   const auto *ASTClass =
@@ -266,6 +281,8 @@ void ASTSrcLocProcessor::run(const MatchFinder::MatchResult &Result) {
     }
   }
 
+  FilterUnsafeAPIs(ClassName, CD);
+
   ClassEntries[ClassName] = CD;
   ClassesInClade[CladeName].push_back(ClassName);
 }

I've been looking through the NodeIntrospection/ASTSrcLocProcessor for quite a few days now and am thoroughly convinced that it is not of the quality/completeness to be a 'maintenance mode' sub-project. If it were under active and continued development, perhaps it would be sufficient 'for now' as it was being developed, but even that I'm not sure would be acceptable.

Additionally, it is incredibly under-tested, resulting in some pretty trivial cases causing failures.

I think the above patch's approach with filtering out the problematic APIs is not the correct direction either, we should be figuring out how to handle those, not just filtering them out ad-hoc.

Based on the above, I'm very much in favor of removing this unless the code quality and test coverage is greatly improved very rapidly. Additionally, it would have to return to active maintenance and development: As it is, it is effectively abandonware.

tl;dr: This is effectively unmaintained abandonware in our repo, we need to FIX all its problems, or remove it ASAP.

I think my llvm-project access was revoked, but if you want to fix the crashes you can apply this patch:

Yeah, we've been updating access control for folks who haven't commit anything in a long time, you probably got hit by that. That said, you should still be able to create a PR and submit it for review (only commit access is revoked, not the ability to submit PRs).

Thank you for the patch, but that really wasn't what we were asking for -- it simply hacks around the problem rather than solving it in a principled way. It's not something we can apply to address this issue.

I spent an hour trying to get to the point I could debug this to see if a proper fix is easy to achieve and was unsuccessful at even getting the introspection data to be generated. (This was despite me adding CMake debugging messages to verify that CLANG_TOOLING_BUILD_AST_INTROSPECTION was set to ON and not reset to OFF later). It seems like the custom commands to run the run-ast-api-dump-tool and run-ast-api-generate-tool are not working, at least for my RelWithDebInfo build using Visual Studio 2022.

Based on the above, I'm very much in favor of removing this unless the code quality and test coverage is greatly improved very rapidly. Additionally, it would have to return to active maintenance and development: As it is, it is effectively abandonware.

As much as I dislike removing work, I am getting more comfortable with this outcome. Unless there's some active, appropriate measures taken to maintain this functionality, I expect to remove it sometime during the Clang 19.x timeframe. That said, if Stephen needs more time to be able to get to that maintenance, I'm happy to be flexible.

the problem rather than solving it in a principled way.

The problem is that some APIs in clang for accessing source locations assert. I suppose you could forbid that but you would have a hard time enforcing that as new APIs are excluded. My pragmatic approach is not unprincipled.

Anyway, I suggest you move on whatever direction you wish!