[NFC] Fix evaluation order dependency in call arguments #141366
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The code assumes parseRegister() executes before the standalone SRegLoc argument to check(). Since function arguments are indeterminately sequenced per C++17 [expr.call]/5, check() may receive a null location instead of the actual parsed source location. Detected by the CFamily analyzer for SonarQube. https://docs.sonarsource.com/sonarqube-cloud/advanced-setup/languages/c-family/overview/
|
@llvm/pr-subscribers-backend-arm Author: Michael Jabbour (michael-jabbour-sonarsource) ChangesThe code in The fix separates the calls to establish explicit sequencing, ensuring This issue was detected by the CFamily analyzer for SonarQube. I'm happy to provide any additional information or clarification as needed. Full diff: https://github.com/llvm/llvm-project/pull/141366.diff 1 Files Affected:
diff --git a/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp b/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
index 85b107c6085d3..ee3dc7d8a3618 100644
--- a/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
+++ b/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
@@ -11788,9 +11788,8 @@ bool ARMAsmParser::parseDirectiveReq(StringRef Name, SMLoc L) {
Parser.Lex(); // Eat the '.req' token.
MCRegister Reg;
SMLoc SRegLoc, ERegLoc;
- if (check(parseRegister(Reg, SRegLoc, ERegLoc), SRegLoc,
- "register name expected") ||
- parseEOL())
+ const bool parseResult = parseRegister(Reg, SRegLoc, ERegLoc);
+ if (check(parseResult, SRegLoc, "register name expected") || parseEOL())
return true;
if (RegisterReqs.insert(std::make_pair(Name, Reg)).first->second != Reg)
|
|
Thanks for the quick review! Feel free to merge this as I don't have commit access myself 😄 |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/88/builds/12116 Here is the relevant piece of the build log for the reference |
michael-jabbour-sonarsource
deleted the
avoid_relying_on_indeterminately_sequenced_eval_order
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The code in
ARMAsmParser::parseDirectiveReqpasses bothparseRegister(Reg, SRegLoc, ERegLoc)andSRegLocas arguments tocheck(). Since function arguments are indeterminately sequenced per C++17 [expr.call]/5, a compiler can evaluateSRegLocbeforeparseRegister()executes. This meanscheck()receives a null location instead of the actual parsed source location for error reporting.The fix separates the calls to establish explicit sequencing, ensuring
check()receives the correct source location.This issue was detected by the CFamily analyzer for SonarQube. I'm happy to provide any additional information or clarification as needed.