This is an archive of the discontinued LLVM Phabricator instance.

Test for a module related crash in CGDebugInfo.cpp
ClosedPublic

Authored by dyung on Apr 12 2016, 8:39 PM.

Details

Summary

In our internal testing, we found a crash in the compiler which was reduced to the attached test case. The issue was "fixed" a while back with Adrian's change in r248069 as the code causing the crash was changed and consequently no longer crashes. The crash occurred in the function CGDebugInfo::EmitImportDecl() in CGDebugInfo.cpp. We did not determine the underlying cause of the crash since it no longer occurs, but wanted to contribute the test case along with some info about it in case anyone might wish to investigate further in case the issue might still exist somewhere.

The function EmitImportDecl contained the following code:

void CGDebugInfo::EmitImportDecl(const ImportDecl &ID) {
    auto *Reader = CGM.getContext().getExternalSource();
    auto Info = Reader->getSourceDescriptor(*ID.getImportedModule());

The crash was because getExternalSource() was returning a null pointer which was assigned into Reader, and when the next line tried to use that pointer, it caused a crash.

Diff Detail

Event Timeline

dyung updated this revision to Diff 53515.Apr 12 2016, 8:39 PM
dyung retitled this revision from to Test for a module related crash in CGDebugInfo.cpp.
dyung updated this object.
dyung added a reviewer: aprantl.
dyung added a subscriber: cfe-commits.

Any thoughts on this test?

aprantl edited edge metadata.Apr 27 2016, 2:27 PM

Thanks!

Is there anything meaningful that could be CHECKed here?
Usually a crash means that we previously had a code path without test coverage.

dyung added a comment.Apr 27 2016, 7:27 PM

Thanks!

Is there anything meaningful that could be CHECKed here?
Usually a crash means that we previously had a code path without test coverage.

This was definitely a code path without test coverage.

When you ask whether there is anything meaningful that could be checked here, are you asking whether we can change the test from testing only that the compilation succeeds to checking something that the compiler produces? If so, I could change the test to produce llvm assembly (-S -emit-llvm) instead, and then perhaps check for the presence of a DIModule within the output. Would that make a better test?

probinson added inline comments.
test/Modules/getSourceDescriptor-crash.cpp
2

Should run %clang_cc1 not the driver.

dyung updated this revision to Diff 55457.Apr 28 2016, 12:32 PM
dyung edited edge metadata.

Modified the test to emit llvm assembly and check for the presence of a DIModule in the output.

dyung updated this revision to Diff 55460.Apr 28 2016, 12:39 PM

Change test to not use the driver.

aprantl added inline comments.Apr 28 2016, 1:22 PM
test/Modules/getSourceDescriptor-crash.cpp
6

This doesn't actually appear in the output :-)
You probably want to at least check for the correct name of the module and for the DIImportedEntity.

dyung added inline comments.Apr 28 2016, 2:10 PM
test/Modules/getSourceDescriptor-crash.cpp
6

Whoops! That was me trying to get it to fail to convince myself it was working and forgot to change it back. I'll restore it and modify the check to what you suggested. Thanks!

dyung updated this revision to Diff 55495.Apr 28 2016, 2:49 PM

Fixed up test and check for that DIImportedEntity and DIModule are present in the generated output.

aprantl accepted this revision.Apr 28 2016, 3:29 PM
aprantl edited edge metadata.
This revision is now accepted and ready to land.Apr 28 2016, 3:29 PM

Please make sure that the two DINodes are connected, otherwise LGTM. Thanks!

This revision was automatically updated to reflect the committed changes.