-
Notifications
You must be signed in to change notification settings - Fork 11.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[IRInterpreter] Return zero address for missing weak function #93548
Conversation
If a weak function is missing, still return it's address (zero) rather than failing interpretation. Otherwise we have a mismatch between Interpret() and CanInterpret() resulting in failures that would not occur with JIT execution. Alternatively, we could try to look for weak symbols in CanInterpret() and generally reject them there. This is the root cause for the issue exposed by llvm#92885. Previously, the case affected by that always fell back to JIT because an icmp constant expression was used, which is not supported by the interpreter. Now a normal icmp instruction is used, which is supported. However, we fail to interpret due to incorrect handling of weak function addresses.
@llvm/pr-subscribers-lldb Author: Nikita Popov (nikic) ChangesIf a weak function is missing, still return it's address (zero) rather than failing interpretation. Otherwise we have a mismatch between Interpret() and CanInterpret() resulting in failures that would not occur with JIT execution. Alternatively, we could try to look for weak symbols in CanInterpret() and generally reject them there. This is the root cause for the issue exposed by Full diff: https://github.com/llvm/llvm-project/pull/93548.diff 1 Files Affected:
diff --git a/lldb/source/Expression/IRInterpreter.cpp b/lldb/source/Expression/IRInterpreter.cpp
index df02922708663..5b670067b5c43 100644
--- a/lldb/source/Expression/IRInterpreter.cpp
+++ b/lldb/source/Expression/IRInterpreter.cpp
@@ -264,7 +264,7 @@ class InterpreterStackFrame {
lldb_private::ConstString name(constant_func->getName());
bool missing_weak = false;
lldb::addr_t addr = m_execution_unit.FindSymbol(name, missing_weak);
- if (addr == LLDB_INVALID_ADDRESS || missing_weak)
+ if (addr == LLDB_INVALID_ADDRESS)
return false;
value = APInt(m_target_data.getPointerSizeInBits(), addr);
return true;
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a reasonable thing to do (as this comment in LoadAddressResolver ::Resolve implies).
Might want to hold off from merging in case @jimingham has any input
It's been a long time since I've looked at this code, but the change to return a 0 value rather than an error seems consistent at least. Was there no way to test this? |
Reapply after #93548, which should address the lldb failure on macos. ----- Do not create icmp/fcmp constant expressions in IRBuilder etc anymore, i.e. treat them as "undesirable". This is in preparation for removing them entirely. Part of: https://discourse.llvm.org/t/rfc-remove-most-constant-expressions/63179
If a weak function is missing, still return it's address (zero) rather than failing interpretation. Otherwise we have a mismatch between Interpret() and CanInterpret() resulting in failures that would not occur with JIT execution.
Alternatively, we could try to look for weak symbols in CanInterpret() and generally reject them there.
This is the root cause for the issue exposed by
#92885. Previously, the case affected by that always fell back to JIT because an icmp constant expression was used, which is not supported by the interpreter. Now a normal icmp instruction is used, which is supported. However, we fail to interpret due to incorrect handling of weak function addresses.