When macros get expanded, the source location for the expanded code received by the Fixable gadgets is invalid. We do not want to emit fixits for macro expanded code and it currently crashes the analysis. This patch fixes the assertion violations that were introduced for handling code with such invalid locations.
Details
Diff Detail
Event Timeline
clang/lib/Analysis/UnsafeBufferUsage.cpp | ||
---|---|---|
1164 | Ideally, we want to return a null to indicate that getExprText failed. Maybe add a FIXME here so that we can fix it later? | |
1348 | If location has no value, this function should return std::nullopt. | |
1491 | If ReplacementLastLoc has no value, we should return an empty FixItList meaning that no fix for the variable declaration. yeah... we are being inconsistent in representing no fix using std::nullopt for Gadgets while using empty list for Decls. We can fix it later. |
clang/lib/Analysis/UnsafeBufferUsage.cpp | ||
---|---|---|
1129 | std::nullopt is a bit more idiomatic as it makes it clear whether the code intends to return an empty optional or a non-empty optional that contains default-initialized (invalid) source location. | |
1161 | * is idiomatic for force-unwrap. | |
1164 | Null StringRef is a thing, you can already use it. But of course callers need to be prepared. Yeah, ideally there needs to be a clear indication that there was an error, because otherwise the callers may assume that the expression is correctly represented by the empty string. As an example of an expression that has a valid reason to be represented by an empty string, default constructors come to mind. Indeed, if C is a class with default constructor, then local variable C c; will have an initializer CXXConstructExpr that doesn't correspond to any text in the source-code. I guess we can run into some of those when we analyze partially spanified code. Of course, ideally by that time we should start using edit generators from libTransformers which already solve these problems. | |
1438–1439 | The FixIts list is never empty; at the very least it unconditionally contains insertion of "{". If LocPassInit turns out to be invalid, we will end up emitting a broken fixit. I think an early return is more appropriate. I think it's safer to do a if(!LocPassInit) return {}; | |
1491 | I think it's safer to write code like if(!ReplacementLastLoc) return {}; so that to make sure that we never emit an incomplete fixit (like we seem to be doing in the other function). |
Looks great for a hotfix! I found a potential problem with new code emitting incomplete fixits which I think we can easily avoid.
Wait nvm, I misunderstood who's saying what, looks like @ziqingluo-90 has already identified the same problem. So there's just one problem.
clang/lib/Analysis/UnsafeBufferUsage.cpp | ||
---|---|---|
1164 | I am not aware of how we can assign Null to a StringRef instance. Do you mean use {} instead of "" ? |
clang/lib/Analysis/UnsafeBufferUsage.cpp | ||
---|---|---|
1164 | An empty StringRef wraps a null pointer. So I think you can just use it. |
clang/lib/Analysis/UnsafeBufferUsage.cpp | ||
---|---|---|
1164 | Ok now you probably want to update all call sites for getExprText() to handle the error, otherwise they may keep crashing or behaving incorrectly. |
clang/lib/Analysis/UnsafeBufferUsage.cpp | ||
---|---|---|
1164 | Maybe we can change the return type of getExprText to std::optional<StringRef> so we the callers can know when it returns an invalid string. |
LGTM! Thanks for fixing the bug!
clang/lib/Analysis/UnsafeBufferUsage.cpp | ||
---|---|---|
1240 | This is irrelevant to this patch: IIRC, the naming convention for variables requires to capitalize the first letter. | |
1272 | Maybe to use getPastLoc instead of getLocWithOffset(1). A problem with Loc.getLocWithOffset(1) is that the returned location always have same FileID as Loc. If Loc is part of a macro but Loc.getLocWithOffset(1) is not suppose to be so, Loc.getLocWithOffset(1) is having a wrong FileID. Locations in macros should have different FileIDs than the ones that are not in a macro. |
clang/lib/Analysis/UnsafeBufferUsage.cpp | ||
---|---|---|
1198 | Ran this through clang-format. | |
1243 | This is unrelated to this patch and seems to be added to satisfy the build bots in this commit:https://github.com/llvm/llvm-project/commit/45a0433b39ffbd7cee9cc8a92f2300324b3548e0. I can address this here or can go as a separate patch. Please let me know. |
LGTM! Thanks for fixing this!
clang/lib/Analysis/UnsafeBufferUsage.cpp | ||
---|---|---|
1243 | Since the fallthrough is not added by this patch we should address it separately. |
std::nullopt is a bit more idiomatic as it makes it clear whether the code intends to return an empty optional or a non-empty optional that contains default-initialized (invalid) source location.