BPF: Emit an error for illegal LD_imm64 insn when LLVM_ENABLE_ASSERTI… #74035
Conversation
This file contains 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
…ONS=OFF Jose reported an issue ([1]) where for the below illegal asm code r0 = 1 + w3 ll clang actually supports it and generates the object code. Further investigation finds that clang actually intends to reject the above code as well but only when the clang is built with LLVM_ENABLE_ASSERTIONS=ON. I later found that clang16 (built by redhat and centos) in fedora system has the same issue since they also have LLVM_ENABLE_ASSERTIONS=OFF ([2]). So let BPF backend report an error for the above case regardless of the LLVM_ENABLE_ASSERTIONS setting. [1] https://lore.kernel.org/bpf/87leahx2xh.fsf@oracle.com/#t [2] https://lore.kernel.org/bpf/840e33ec-ea4c-4b55-bda1-0be8d1e0324f@linux.dev/
|
I tried building with |
|
Also I'm not sure if simply asserting is good in this context. I think a better approach is to:
|
@eddyz87 The following is my cmake command line: And confirms assertion is indeed off with the build Top commit The test Did I miss anything? |
|
That's what I did, yes. Let me rebuild once again, just in case. |
|
Rebuilt everything from scratch using the following commands: Confirmed that assertions are off: Still no error: Repository state: |
|
It is very strange. I am using the same cmake options. The top commits in my repo where the top commit is this patch. Still I got the same failure. My previous initial compiler is not specified and it uses 'gcc' 8.5 on the host. @4ast, @anakryiko if you got time, could you try as well with your setup with this patch and LLVM_ENABLE_ASSERTIONS=OFF? @eddyz87 Could you debug why it didn't error out? That is, why the code path didn't reach printExpr() function and triggers an error? |
|
For me, when assertions are enabled, the error is reported not in Subsequently, the following modification works with both assertions on and off: diff --git a/llvm/lib/Target/BPF/MCTargetDesc/BPFMCCodeEmitter.cpp b/llvm/lib/Target/BPF/MCTargetDesc/BPFMCCodeEmitter.cpp
index b807d6904004..4f4ebdabf810 100644
--- a/llvm/lib/Target/BPF/MCTargetDesc/BPFMCCodeEmitter.cpp
+++ b/llvm/lib/Target/BPF/MCTargetDesc/BPFMCCodeEmitter.cpp
@@ -89,6 +89,14 @@ unsigned BPFMCCodeEmitter::getMachineOpValue(const MCInst &MI,
const MCExpr *Expr = MO.getExpr();
+ if (Expr->getKind() != MCExpr::SymbolRef) {
+ std::string Msg;
+ llvm::raw_string_ostream S(Msg);
+ S << "getMachineOpValue: unexpected Expr->getKind() == "
+ << (unsigned)Expr->getKind() << ": "
+ << "'" << *Expr << "'" << "\n";
+ report_fatal_error(StringRef(Msg));
+ }
assert(Expr->getKind() == MCExpr::SymbolRef);
if (MI.getOpcode() == BPF::JAL)Still, I think that error should be reported earlier, when expressions are parsed. |
|
Okay, my crash call stack: |
The above code is in BPFMCCodeEmitter which is later than my crash point, so I suspect it won't affect my crash signature. Indeed, I applied the above change and my crash pattern remains the same. |
|
I cherry-picked your commit I apologize for causing this confusion. I can approve this change if you don't plan to update it to report error on parsing stage. |
|
No problem, it is still a mystery why old build interferes with new build, I suppose cmake should take care of this. But anyway, glad now we are on the same page. I think the current change is okay and I don't plan to check other asserts at this moment. Those asserts are mostly for BPF llvm backend developers. I assume most (if not all) users should be able to write inline asm with correct syntax if they intend to embed inline asm in their bpf programs. So please stamp if you get a chance. Thanks! |
eddyz87
approved these changes
Dec 7, 2023
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.
…ONS=OFF
Jose reported an issue ([1]) where for the below illegal asm code
clang actually supports it and generates the object code.
Further investigation finds that clang actually intends to reject the above code as well but only when the clang is built with LLVM_ENABLE_ASSERTIONS=ON.
I later found that clang16 (built by redhat and centos) in fedora system has the same issue since they also have LLVM_ENABLE_ASSERTIONS=OFF ([2]).
So let BPF backend report an error for the above case regardless of the LLVM_ENABLE_ASSERTIONS setting.
[1] https://lore.kernel.org/bpf/87leahx2xh.fsf@oracle.com/#t
[2] https://lore.kernel.org/bpf/840e33ec-ea4c-4b55-bda1-0be8d1e0324f@linux.dev/