This is an archive of the discontinued LLVM Phabricator instance.

[libclang] Record parsing invocation to a temporary file when requested by client
ClosedPublic

Authored by arphaman on Nov 27 2017, 3:12 PM.

Details

Summary

This patch extends libclang by allowing it to record parsing operations to a temporary JSON file. The file is deleted after parsing succeeds. When a crash happens during parsing, the file is preserved and the client will be able to use it to generate a reproducer for the crash.

These files are not emitted by default, and the client has to specify the invocation emission path first.

rdar://35322543

Diff Detail

Repository
rL LLVM

Event Timeline

arphaman created this revision.Nov 27 2017, 3:12 PM
nik added a subscriber: nik.Nov 28 2017, 12:10 AM

Could you elaborate on "the client will be able to use it to generate a reproducer for the crash"? Having the json file, what would I need to do in order to reproduce the crash?

nik added a subscriber: yvvan.Nov 28 2017, 1:05 AM
arphaman added a comment.EditedNov 28 2017, 4:51 PM
In D40527#937282, @nik wrote:

Could you elaborate on "the client will be able to use it to generate a reproducer for the crash"? Having the json file, what would I need to do in order to reproduce the crash?

Sure. I would like to teach clang how to interpret this file so that it can generate a reproducer for a crash (similar to a -gen-reproducer) option in clang. This way external IDE clients could leverage clang's reproducer infrastructure to generate reproducers for libclang crashes. The external client will be responsible for determining when a crash happens and what JSON files have to be passed to clang.

jkorous-apple added inline comments.Nov 29 2017, 7:10 AM
tools/libclang/CIndexer.cpp
131 ↗(On Diff #124480)

Just a thought - since we are not propagating errors from constructor we are not really sure we were able to create the file in the first place (e. g. return from ctor at line 103). Should we still try to delete it?

jkorous-apple added inline comments.Nov 29 2017, 7:54 AM
tools/libclang/CIndexer.cpp
108 ↗(On Diff #124480)

Nit: Maybe capturing &OS is enough?

arphaman marked an inline comment as done.Nov 29 2017, 11:28 AM
arphaman added inline comments.
tools/libclang/CIndexer.cpp
131 ↗(On Diff #124480)

If createUniqueFile failed in the constructor (return on line 103) then File will be empty. Therefore we won't try to remove the file.

arphaman updated this revision to Diff 124788.Nov 29 2017, 11:29 AM
  • Capture OS only.
  • Guard toolchain path accessor with a mutex.
jkorous-apple edited edge metadata.Dec 4 2017, 6:11 AM

LGTM otherwise.

tools/libclang/CIndexer.h
45 ↗(On Diff #124788)

I am just wondering - since we anticipate multi-threaded usage, shouldn't we synchronize access to all member variables mutable via public class interface?

Specifically - shouldn't we use one mutex per every mutating/reading method group?

  • setInvocationEmissionPath(), getInvocationEmissionPath()
  • setCXGlobalOptFlags(), isOptEnabled()
  • getClangResourcesPath()
arphaman added inline comments.Dec 4 2017, 12:58 PM
tools/libclang/CIndexer.h
45 ↗(On Diff #124788)

Actually CIndex is not thread safe, so we don't need the mutex. I'll remove it.

This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.