Page MenuHomePhabricator

Instantiate Error in Target::GetEntryPointAddress() only when necessary
ClosedPublic

Authored by dim on Aug 21 2020, 8:50 AM.

Details

Summary

When Target::GetEntryPointAddress() calls exe_module->GetObjectFile()->GetEntryPointAddress(), and the returned
entry_addr is valid, it can immediately be returned.

However, just before that, an llvm::Error value has been setup, but in this case it is not consumed before returning, like is done further below in the function.

In https://bugs.freebsd.org/248745 we got a bug report for this, where a very simple test case aborts and dumps core:

* thread #1, name = 'testcase', stop reason = breakpoint 1.1
    frame #0: 0x00000000002018d4 testcase`main(argc=1, argv=0x00007fffffffea18) at testcase.c:3:5
   1	int main(int argc, char *argv[])
   2	{
-> 3	    return 0;
   4	}
(lldb) p argc
Program aborted due to an unhandled Error:
Error value was Success. (Note: Success values must still be checked prior to being destroyed).

Thread 1 received signal SIGABRT, Aborted.
thr_kill () at thr_kill.S:3
3	thr_kill.S: No such file or directory.
(gdb) bt
#0  thr_kill () at thr_kill.S:3
#1  0x00000008049a0004 in __raise (s=6) at /usr/src/lib/libc/gen/raise.c:52
#2  0x0000000804916229 in abort () at /usr/src/lib/libc/stdlib/abort.c:67
#3  0x000000000451b5f5 in fatalUncheckedError () at /usr/src/contrib/llvm-project/llvm/lib/Support/Error.cpp:112
#4  0x00000000019cf008 in GetEntryPointAddress () at /usr/src/contrib/llvm-project/llvm/include/llvm/Support/Error.h:267
#5  0x0000000001bccbd8 in ConstructorSetup () at /usr/src/contrib/llvm-project/lldb/source/Target/ThreadPlanCallFunction.cpp:67
#6  0x0000000001bcd2c0 in ThreadPlanCallFunction () at /usr/src/contrib/llvm-project/lldb/source/Target/ThreadPlanCallFunction.cpp:114
#7  0x00000000020076d4 in InferiorCallMmap () at /usr/src/contrib/llvm-project/lldb/source/Plugins/Process/Utility/InferiorCallPOSIX.cpp:97
#8  0x0000000001f4be33 in DoAllocateMemory () at /usr/src/contrib/llvm-project/lldb/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp:604
#9  0x0000000001fe51b9 in AllocatePage () at /usr/src/contrib/llvm-project/lldb/source/Target/Memory.cpp:347
#10 0x0000000001fe5385 in AllocateMemory () at /usr/src/contrib/llvm-project/lldb/source/Target/Memory.cpp:383
#11 0x0000000001974da2 in AllocateMemory () at /usr/src/contrib/llvm-project/lldb/source/Target/Process.cpp:2301
#12 CanJIT () at /usr/src/contrib/llvm-project/lldb/source/Target/Process.cpp:2331
#13 0x0000000001a1bf3d in Evaluate () at /usr/src/contrib/llvm-project/lldb/source/Expression/UserExpression.cpp:190
#14 0x00000000019ce7a2 in EvaluateExpression () at /usr/src/contrib/llvm-project/lldb/source/Target/Target.cpp:2372
#15 0x0000000001ad784c in EvaluateExpression () at /usr/src/contrib/llvm-project/lldb/source/Commands/CommandObjectExpression.cpp:414
#16 0x0000000001ad86ae in DoExecute () at /usr/src/contrib/llvm-project/lldb/source/Commands/CommandObjectExpression.cpp:646
#17 0x0000000001a5e3ed in Execute () at /usr/src/contrib/llvm-project/lldb/source/Interpreter/CommandObject.cpp:1003
#18 0x0000000001a6c4a3 in HandleCommand () at /usr/src/contrib/llvm-project/lldb/source/Interpreter/CommandInterpreter.cpp:1762
#19 0x0000000001a6f98c in IOHandlerInputComplete () at /usr/src/contrib/llvm-project/lldb/source/Interpreter/CommandInterpreter.cpp:2760
#20 0x0000000001a90b08 in Run () at /usr/src/contrib/llvm-project/lldb/source/Core/IOHandler.cpp:548
#21 0x00000000019a6c6a in ExecuteIOHandlers () at /usr/src/contrib/llvm-project/lldb/source/Core/Debugger.cpp:903
#22 0x0000000001a70337 in RunCommandInterpreter () at /usr/src/contrib/llvm-project/lldb/source/Interpreter/CommandInterpreter.cpp:2946
#23 0x0000000001d9d812 in RunCommandInterpreter () at /usr/src/contrib/llvm-project/lldb/source/API/SBDebugger.cpp:1169
#24 0x0000000001918be8 in MainLoop () at /usr/src/contrib/llvm-project/lldb/tools/driver/Driver.cpp:675
#25 0x000000000191a114 in main () at /usr/src/contrib/llvm-project/lldb/tools/driver/Driver.cpp:890

Fix the incorrect error catch by only instantiating an Error object if it is necessary.

Diff Detail

Event Timeline

dim created this revision.Aug 21 2020, 8:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 21 2020, 8:50 AM
dim requested review of this revision.Aug 21 2020, 8:50 AM
JDevlieghere added inline comments.
lldb/source/Target/Target.cpp
2408–2442

I'm not particularly happy with the error handling in this function. The patch seems correct, but as I've demonstrated it's easy to get this wrong. I think we can rewrite the method as suggested to eliminate the issue. What do you think?

dim added inline comments.Aug 21 2020, 11:49 AM
lldb/source/Target/Target.cpp
2408–2442

Yeah, I think that is much less error-prone. The consumeError method has a relevent comment:

/// Uses of this method are potentially indicative of problems: perhaps the
/// error should be propagated further, or the error-producer should just
/// return an Optional in the first place.

In this case it was just problematic to instantiate an Error right at the start, instead of when an actual error condition occurred.

So I agree that your proposed approach is better. There is still one consumeError() in there, though?

dim updated this revision to Diff 287077.Aug 21 2020, 12:04 PM

As @JDevlieghere suggests, only instantiate Error objects when necessary.

dim retitled this revision from Consume error for valid return from Target::GetEntryPointAddress() to Instantiate Error in Target::GetEntryPointAddress() only when necessary.Aug 21 2020, 12:06 PM
dim edited the summary of this revision. (Show Details)
JDevlieghere accepted this revision.Aug 21 2020, 3:32 PM

LGTM

lldb/source/Target/Target.cpp
2408–2442

There is still one consumeError() in there, though?

I don't think that's true anymore?

This revision is now accepted and ready to land.Aug 21 2020, 3:32 PM
This revision was landed with ongoing or failed builds.Aug 22 2020, 3:47 AM
This revision was automatically updated to reflect the committed changes.