This is an archive of the discontinued LLVM Phabricator instance.

Fix process launch failure on FreeBSD after r365761
ClosedPublic

Authored by dim on Oct 9 2019, 1:08 PM.

Details

Summary

After rLLDB365761, and with LLVM_ENABLE_ABI_BREAKING_CHECKS enabled, launching
any process on FreeBSD crashes lldb with:

Expected<T> must be checked before access or destruction.
Expected<T> value was in success state. (Note: Expected<T> values in success mode must still be checked prior to being destroyed).

This is because m_operation_thread and m_monitor_thread were wrapped in
llvm::Expected<>, but this requires the objects to be correctly initialized
before accessing them.

To fix the crashes, revert the wrapping, and use local variables to store the
return values of LaunchThread and StartMonitoringChildProcess. Then, only
assign to the member variables after checking if the return values indicated
success.

Diff Detail

Event Timeline

dim created this revision.Oct 9 2019, 1:08 PM
dim added a comment.Oct 9 2019, 1:09 PM
This comment was removed by dim.

This was probably due to:

  Expected(Error Err)
      : HasError(true)
#if LLVM_ENABLE_ABI_BREAKING_CHECKS
        // Expected is unchecked upon construction in Debug builds.
        , Unchecked(true)
#endif
  {
    assert(Err && "Cannot create Expected<T> from Error success value.");
    new (getErrorStorage()) error_type(Err.takePayload());
  }

The member initialization makes the members unchecked m_operation_thread(nullptr). Adding something like (void)!m_operation_thread before assignment to m_operation_thread is probably better.

devnexen added inline comments.Oct 9 2019, 11:28 PM
source/Plugins/Process/FreeBSD/ProcessMonitor.cpp
733 ↗(On Diff #224137)

nit: Would prefer obvious typing but can go along with it, just a detail.

devnexen accepted this revision.Oct 9 2019, 11:29 PM

LGTM otherwise.

This revision is now accepted and ready to land.Oct 9 2019, 11:29 PM

LGTM otherwise.

I don't think this change should be reverted. It can just be repaired by adding some (void)!xxx;

labath added a subscriber: labath.Oct 10 2019, 1:42 AM

I don't think having Expected<T> members is a reasonable thing to do. If we want to keep the notion of validity explicit (which is consistent with the direction lldb is moving in), then I think the members should be Optional<T>. The initialization code can take the Expected<T> result, do something with the error, and then emplace the result into the Optional<T> member in case of success.

LGTM otherwise.

I don't think this change should be reverted. It can just be repaired by adding some (void)!xxx;

LGTM otherwise.

I don't think this change should be reverted. It can just be repaired by adding some (void)!xxx;

I would prefer this approach indeed but either way I m in that s a real issue at the moment since LLVM runtime check is on in the FreeBSD's system.

@dim I LGTMed this but would it a real issue for you to take on a less disruptive approach proposed by MaskRay ?

@dim I LGTMed this but would it a real issue for you to take on a less disruptive approach proposed by MaskRay ?

I think we should do what @labath suggested (changing Expected to Optional) if that is not very difficult.

@dim I LGTMed this but would it a real issue for you to take on a less disruptive approach proposed by MaskRay ?

I think we should do what @labath suggested (changing Expected to Optional) if that is not very difficult.

Good point, I missed it. Should be alright to.

dim updated this revision to Diff 224447.Oct 10 2019, 12:29 PM

Convert m_(monitor|operation)_thread to llvm::Optional<>.

devnexen accepted this revision.Oct 10 2019, 12:50 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 10 2019, 1:27 PM