Prevent infinite recursive loop in AppleObjCTrampolineHandler constructor

[fjricci]

Summary

When we construct AppleObjCTrampolineHandler, if m_impl_fn_addr is invalid, we call CanJIT(). If the gdb remote process does not support allocating and deallocating memory, this call stack will include a call to the AppleObjCRuntime constructor. The AppleObjCRuntime constructor will then call the AppleObjCTrampolineHandler constructor, creating a recursive call loop that eventually overflows the stack and segfaults.

Avoid this call loop by not constructing the AppleObjCTrampolineHandler within AppleObjCRuntime until we actually need to use it.

[jingham]

Can you post the backtrace in this case? It seems to me odd that CanJIT -> AllocateMemory needs to construct the AppleObjCRuntime to do its job. I'd rather cut the chain at that point if it makes sense.

[fjricci]

Here's a paste of the end of the backtrace.

pastebin.com/3VkF3Biq

[jingham]

Ah, right. We're doing that so that we can make sure that an ObjC Exception thrown during the course of expression evaluation doesn't unwind past the frame we are using to call the function.

Seems like for low-level stuff like AllocateMemory we should be avoiding all this extra work, which you can do by calling options.SetTrapExceptions(false) on the ExpressionEvaluationOptions that you pass in to the thread plan in InferiorCallMmap in InferiorCallPOSIX.cpp.

Would you mind checking that adding that bit to InferiorCallMmap (without your change to avoid recursion fixes the problem as well.) I don't mind the current change as well, but it would be really nice to get the ObjC runtime out of the picture when trying to allocate memory...

[jingham]

That penultimate sentence would be clearer if I had put the end ")" in the right place - after "recursion"

[fjricci]

Follow suggestion by @jingham to avoid setting up ObjC runtime for low-level POSIX memory allocations

[fjricci]

@jingham - your suggestion does avoid the recursion as well, and seems a lot cleaner.

[jingham]

Cool! For completeness I think it is a good idea to do this to all the functions in InferiorCallPOSIX. For the general InferiorCall one it might be a good idea to add a defaulted (to false) parameter so that if somebody really needs to use it for something that might throw they can. But this is for calling simple low level function which shouldn't be throwing, so defaulting to not firing up the runtimes to catch exceptions is more appropriate.

Then this is good to go.

[fjricci]

Would you still like me to make the parameter change to InferiorCallMmap before merging?

[jingham]

If you don't mind, please add the same code to the InferiorCallMunmap, and then add the same thing to InferiorCall, but that's the one that should take a parameter in case somebody does end up using the general function for something that could throw. Otherwise we'll just end up coming back to this again later...

[fjricci]

Sounds good, will do.

[fjricci]

Disable exception trapping by default on all functions in InferiorCallPOSIX

[jingham]

Great, thanks!

[sas]

Committed as r257204.

source/Plugins/Process/Utility/InferiorCallPOSIX.h

 };
 
 bool InferiorCallMmap(Process *proc, lldb::addr_t &allocated_addr,
                       lldb::addr_t addr, lldb::addr_t length, unsigned prot,
                       unsigned flags, lldb::addr_t fd, lldb::addr_t offset);
 
 bool InferiorCallMunmap(Process *proc, lldb::addr_t addr, lldb::addr_t length);
 
-bool InferiorCall(Process *proc, const Address *address, lldb::addr_t &returned_func);
+bool InferiorCall(Process *proc, const Address *address, lldb::addr_t &returned_func,
+                  bool trap_exceptions = false);
 
 }   // namespace lldb_private
 
 #endif  // lldb_InferiorCallPOSIX_h_

source/Plugins/Process/Utility/InferiorCallPOSIX.cpp

if (count > 0)
             const bool use_inline_block_range = false;
             EvaluateExpressionOptions options;
             options.SetStopOthers(true);
             options.SetUnwindOnError(true);
             options.SetIgnoreBreakpoints(true);
             options.SetTryAllThreads(true);
             options.SetDebug (false);
             options.SetTimeoutUsec(500000);
+            options.SetTrapExceptions(false);
 
             addr_t prot_arg, flags_arg = 0;
             if (prot == eMmapProtNone)
               prot_arg = PROT_NONE;
             else {
               prot_arg = 0;
               if (prot & eMmapProtExec)
                 prot_arg |= PROT_EXEC;
if (count > 0)
             const bool use_inline_block_range = false;
             EvaluateExpressionOptions options;
             options.SetStopOthers(true);
             options.SetUnwindOnError(true);
             options.SetIgnoreBreakpoints(true);
             options.SetTryAllThreads(true);
             options.SetDebug (false);
             options.SetTimeoutUsec(500000);
+            options.SetTrapExceptions(false);
            
             AddressRange munmap_range;
             if (sc.GetAddressRange(range_scope, 0, use_inline_block_range, munmap_range))
             {
                 lldb::addr_t args[] = { addr, length };
                 lldb::ThreadPlanSP call_plan_sp (new ThreadPlanCallFunction (*thread,
                                                                             munmap_range.GetBaseAddress(),
                                                                             CompilerType(),

 }
 
 // FIXME: This has nothing to do with Posix, it is just a convenience function that calls a
 // function of the form "void * (*)(void)".  We should find a better place to put this.
 
 bool
 lldb_private::InferiorCall (Process *process,
                             const Address *address,
-                            addr_t &returned_func)
+                            addr_t &returned_func,
+                            bool trap_exceptions)
 {
     Thread *thread = process->GetThreadList().GetSelectedThread().get();
     if (thread == NULL || address == NULL)
         return false;
 
     EvaluateExpressionOptions options;
     options.SetStopOthers(true);
     options.SetUnwindOnError(true);
     options.SetIgnoreBreakpoints(true);
     options.SetTryAllThreads(true);
     options.SetDebug (false);
     options.SetTimeoutUsec(500000);
+    options.SetTrapExceptions(trap_exceptions);
 
     ClangASTContext *clang_ast_context = process->GetTarget().GetScratchClangASTContext();
     CompilerType clang_void_ptr_type = clang_ast_context->GetBasicType(eBasicTypeVoid).GetPointerType();
     lldb::ThreadPlanSP call_plan_sp (new ThreadPlanCallFunction (*thread,
                                                                  *address,
                                                                  clang_void_ptr_type,
                                                                  llvm::ArrayRef<addr_t>(),
                                                                  options));