Fix "ninja check-lldb" crash in IRExecutionUnit.cpp

[ted]

Summary

From Adrian McCarthy:

"Running ninja check-lldb now has one crash in a Python process, due to deferencing a null pointer in IRExecutionUnit.cpp: candidate_sc.symbol is null, which leads to a call with a null this pointer."

[amccarth]

Thanks for the fix.

Right now I'm trying to figure out why the test framework didn't detect the crash and reported that all the tests passed.

source/Expression/IRExecutionUnit.cpp

801

I'm OK with this, but I believe the LLDB style is to reverse the logic and use an early-out (a continue in this case) rather than increasing the depth of the rest of the code.

[spyffe]

Looks good to me. Thanks for the fix!

source/Expression/IRExecutionUnit.cpp

805–812

Ah yes, I see what's being fixed here. This looks fine. I like Adrian's recommended early-out policy but this isn't a hard-and-fast rule. It seems like doing the logic the way you're doing it is less invasive, so I'm fine with doing it this way for now.

[jingham]

I don't think this is right. It is possible to have a sc.symbol be nullptr, but sc.function be valid. So the check for sc.symbol will reject the valid information in the function. Note, the code in the function is also wrong, since it only gets the address from the symbol.

[jingham]

Greg suggests adding something to SymbolContext to get the start address of the function that handles this possibility. GetAddressRange sort of does this, but only if the SymbolContext doesn't have a block or line entry.

[ted]

Updated to use early-out as requested

[ted]

The change is to guard against the case where candidate_sc.symbol is nullptr.

candidate_sc.function is only used when load_address != LLDB_INVALID_ADDRESS, but load_address is set on line 802:

load_address = candidate_sc.symbol->ResolveCallableAddress(*target);

so candidate_sc.symbol must be valid.

The purpose of the function is to get the address of a symbol, so I don't think we care about candidate_sc.function when candidate_sc.symbol is nullptr.

[ted]

So you'd like to see this function get the address of a function that it
finds in either symbols or debug info?

Which should it prioritize when we have both?

[ted]

Updated to address Jim's comments.

[jingham]

That seems fine to me. Thanks for putting up with the niggling...

source/Expression/IRExecutionUnit.cpp

for (const SearchSpec &spec : specs)
             for (size_t si = 0, se = sc_list.GetSize(); si < se; ++si)
             {
                 SymbolContext candidate_sc;
 
                 sc_list.GetContextAtIndex(si, candidate_sc);
 
                 const bool is_external = (candidate_sc.function) ||
                                          (candidate_sc.symbol && candidate_sc.symbol->IsExternal());
-
+                if (candidate_sc.symbol)
+                {
                     load_address = candidate_sc.symbol->ResolveCallableAddress(*target);
 
                     if (load_address == LLDB_INVALID_ADDRESS)
                     {
                         if (target->GetProcessSP())
                             load_address = candidate_sc.symbol->GetAddress().GetLoadAddress(target);
                         else
                             load_address = candidate_sc.symbol->GetAddress().GetFileAddress();
                     }
+                }
+
+                if (load_address == LLDB_INVALID_ADDRESS && candidate_sc.function)
+                {
+                        if (target->GetProcessSP())
+                            load_address = candidate_sc.function->GetAddressRange().GetBaseAddress().GetLoadAddress(target);
+                        else
+                            load_address = candidate_sc.function->GetAddressRange().GetBaseAddress().GetFileAddress();
+                }
 
                 if (load_address != LLDB_INVALID_ADDRESS)
                 {
                     if (is_external)
                     {
                         return true;
                     }
                     else if (best_internal_load_address == LLDB_INVALID_ADDRESS)