This is an archive of the discontinued LLVM Phabricator instance.

[Reproducers] Capture return values of functions returning by ptr/ref
ClosedPublic

Authored by JDevlieghere on Apr 2 2019, 9:33 PM.

Details

Summary

For some reason I had convinced myself that functions returning by pointer or reference do not require recording their result. However, after further considering I don't see how that could work, at least not with the current implementation. Interestingly enough, the reproducer instrumentation already (mostly) accounts for this, though the lldb-instr tool did not.

This patch adds the missing macros and updates the lldb-instr tool.

Diff Detail

Event Timeline

JDevlieghere created this revision.Apr 2 2019, 9:33 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: abidh. · View Herald Transcript
JDevlieghere marked 2 inline comments as done.Apr 2 2019, 9:34 PM
JDevlieghere added inline comments.
lldb/include/lldb/Utility/ReproducerInstrumentation.h
679

Let's cast this or hide it behind a macro to prevent code duplication.

lldb/unittests/Utility/ReproducerInstrumentationTest.cpp
552

Oops, this should be removed.

labath added a comment.Apr 3 2019, 1:10 AM

I agree that these functions should be recorded. I think the reason that the infrastructure already mostly supports that is that we were modelling constructors as functions that return pointers (and you definitely need to record constructors).

lldb/include/lldb/Utility/ReproducerInstrumentation.h
678–716

I would expect that all of these could be replaced with one function using universal references. I.e., something like:

template<typename Result> Result &&RecordResult(Result &&r) {
...
m_serializer.SerializeAll(r); // No std::forward, as you don't want to move r into this function
...
return std::forward<Result>(r);
}

Can you try that out?

  • Add tests for lldb-instr
  • Feedback Pavel
friss accepted this revision.Apr 3 2019, 2:08 PM

This seems super mechanical and we discussed it at length offline. LGTM

This revision is now accepted and ready to land.Apr 3 2019, 2:08 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 3 2019, 2:32 PM