Page MenuHomePhabricator

[libclang] Expose abort()-ing fatal error handler
ClosedPublic

Authored by jkorous on Aug 26 2019, 4:59 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

jkorous created this revision.Aug 26 2019, 4:59 PM

Is there any way to test this, or is it pretty much hopeless?

jkorous abandoned this revision.Aug 28 2019, 4:00 PM

How to minimize a patch to nothing:

  1. Write unit test (EXPECT_DEATH during parsing of #pragma clang __debug parser_crash).
  2. Hit CrashRecoveryContext.
  3. Find clang_toggleCrashRecovery.

I guess what we were looking for is clang_toggleCrashRecovery(0);.

Is there any way to test this, or is it pretty much hopeless?

How to minimize a patch to nothing:

  1. Write unit test (EXPECT_DEATH during parsing of #pragma clang __debug parser_crash).
  2. Hit CrashRecoveryContext.
  3. Find clang_toggleCrashRecovery.

    I guess what we were looking for is clang_toggleCrashRecovery(0);.

Crash recovery is completely orthogonal! It just happens in your constructed test case, not all APIs go through crash recovery. Please re-open the patch.

jkorous reclaimed this revision.Sep 3 2019, 1:59 PM
jkorous updated this revision to Diff 218539.Sep 3 2019, 3:01 PM
jkorous edited the summary of this revision. (Show Details)

Added a proper test.

It turned out that recovery context matters as it messes with signals.

jkorous updated this revision to Diff 218541.Sep 3 2019, 3:17 PM

There are actually couple other fatal-error-handling strategies. But all the other I've seen raise a signal (mostly SIGABRT; SIGILL in case of LLVM_BUILTIN_TRAP). For the purpose of having a way how to ask libclang to crash in a way that might lead to core dump this seems fine. Uninstalling the handler doesn't lead to libclang just calling exit in case of any crash though. Maybe I should rename the functions? Open to suggestions...

jkorous planned changes to this revision.Sep 3 2019, 3:44 PM
jkorous requested review of this revision.Sep 3 2019, 4:17 PM
arphaman added inline comments.Sep 4 2019, 12:19 PM
clang/include/clang-c/FatalErrorHandler.h
20 ↗(On Diff #218541)

What is the purpose of this FIXME? Will it be needed to install fatal error handler for other kinds of LLVM traps/calls?

jkorous marked an inline comment as done.Sep 4 2019, 3:56 PM
jkorous added inline comments.
clang/include/clang-c/FatalErrorHandler.h
20 ↗(On Diff #218541)

Sorry, I didn't make it clear. It's related to what I wrote in the change description:

There are actually couple other fatal-error-handling strategies. But all the other I've seen raise a signal (mostly SIGABRT; SIGILL in case of LLVM_BUILTIN_TRAP). For the purpose of having a way how to ask libclang to crash in a way that might lead to core dump this seems fine. Uninstalling the handler doesn't lead to libclang just calling exit in case of any crash though. Maybe I should rename the functions? Open to suggestions...

In other words - I am not too excited about creating an interface whose only function is to expose "detail of an implementation detail". On the other hand - I don't see a practical way how to implement a libclang-wide error handling strategy. The best I can imagine is naming these functions in weird-enough way that nobody would assume they promise more than they actually do. WDYT?

arphaman accepted this revision.Sep 4 2019, 4:18 PM
arphaman added inline comments.
clang/include/clang-c/FatalErrorHandler.h
20 ↗(On Diff #218541)

Yes, unfortunately it's not an ideal interface. There's really no way for us to know what the client wants though, so I think this would have to be a reasonable compromise :(

I think it might be better to rename it to clang_install_aborting_llvm_fatal_error_handler to make it more clear that it's only for the LLVM fatal errors.

LGTM otherwise, thanks!

This revision is now accepted and ready to land.Sep 4 2019, 4:18 PM
ddunbar added inline comments.Sep 4 2019, 8:29 PM
clang/include/clang-c/FatalErrorHandler.h
20 ↗(On Diff #218541)

Makes sense to me.

clang/tools/libclang/CIndex.cpp
3249 ↗(On Diff #218541)

I don't get it. Per the ManagedStatic below, it looks like libclang was already trying to always install the handler. But based on the behavior I observed, it looks like some uses cases weren't necessarily triggering it.

It feels strange to add these new APIs when the ManagedStatic below will I think mean that the installation automatically happens?

jkorous marked an inline comment as done.Sep 6 2019, 5:38 PM
jkorous added inline comments.
clang/tools/libclang/CIndex.cpp
3249 ↗(On Diff #218541)

IIUC you need to call operator*() on managed ManagedStatic to have it construct an instance of the underlying type.

https://llvm.org/doxygen/classllvm_1_1ManagedStatic.html
"This transparently changes the behavior of global statics to be lazily constructed on demand"

https://llvm.org/doxygen/ManagedStatic_8h_source.html#l00083

That does happens in clang_createIndex:

// Look through the managed static to trigger construction of the managed
// static which registers our fatal error handler. This ensures it is only
// registered once.
(void)*RegisterFatalErrorHandlerOnce;

but not in other parts of the API. Now, some other functions expects CXIndex as an input which would imply the handler is set up before you can call them but that's still just a subset of the whole libclang API.

jkorous updated this revision to Diff 219209.Sep 6 2019, 5:51 PM
  • renamed functions to suggest their limitations
  • added one more test
ddunbar accepted this revision.Sep 12 2019, 3:29 PM
ddunbar added inline comments.
clang/tools/libclang/CIndex.cpp
3249 ↗(On Diff #218541)

Got it, that makes sense now.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 12 2019, 3:56 PM

This breaks one of the clang unit tests on Fedora 30 (x86_64). Can we revert or fix this?

FAIL: Clang-Unit :: libclang/CrashTests/./libclangCrashTests/LibclangParseTest.UninstallAbortingLLVMFatalErrorHandler (19745 of 51413)
******************** TEST 'Clang-Unit :: libclang/CrashTests/./libclangCrashTests/LibclangParseTest.UninstallAbortingLLVMFatalErrorHandler' FAILED ********************
Note: Google Test filter = LibclangParseTest.UninstallAbortingLLVMFatalErrorHandler
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from LibclangParseTest
[ RUN      ] LibclangParseTest.UninstallAbortingLLVMFatalErrorHandler
LLVM ERROR: #pragma clang __debug llvm_fatal_error

********************
Testing Time: 86.35s
********************
Failing Tests (1):
    Clang-Unit :: libclang/CrashTests/./libclangCrashTests/LibclangParseTest.UninstallAbortingLLVMFatalErrorHandler

  Expected Passes    : 40666
  Expected Failures  : 84
  Unsupported Tests  : 10662
  Unexpected Failures: 1
thakis added inline comments.
cfe/trunk/unittests/libclang/CrashTests/CMakeLists.txt
1

Can you add a comment on why this isn't just in libclangTests? I'm guessing it's to make sure the global error handler state doesn't accidentally link into the other tests in libclangTests, but I'm not sure.

aaronpuchert added inline comments.
cfe/trunk/tools/libclang/CMakeLists.txt
47

Why is this needed?

113

The support library is already added here.