Page MenuHomePhabricator

[DebugInfo] Add debug location to stubs generated by CGDeclCXX and mark them as artificial
ClosedPublic

Authored by aganea on Aug 15 2019, 7:26 PM.

Details

Summary

Previously, when clang was compiled with -DLLVM_ENABLE_ASSERTIONS=ON, the attached test was yielding:

inlinable function call in a function with debug info must have a !dbg location
  call void @"??1?$c@UB@@@@QEAA@XZ"(%struct.c* @"?f@?1??d@@YAPEAU?$c@UB@@@@XZ@4U2@A")
fatal error: error in backend: Broken module found, compilation aborted!
Stack dump:
0.      Program arguments: <f:\svn\buildninja\bin\clang -cc1 -emit-llvm debug-info-no-location.cpp> -gcodeview -debug-info-kind=limited
1.      <eof> parser at end of file
2.      Per-function optimization

Fixes PR43012

Diff Detail

Event Timeline

aganea created this revision.Aug 15 2019, 7:26 PM
aganea edited the summary of this revision. (Show Details)
aprantl requested changes to this revision.Aug 16 2019, 9:10 AM
aprantl added inline comments.
test/CodeGen/debug-info-no-location.cpp
4 ↗(On Diff #215526)

Please don't hardcode metadata numbers in the tests, this is bound to break almost immediately.

It seems like this function is compiler-generated and thus should be marked as artificial?

This revision now requires changes to proceed.Aug 16 2019, 9:10 AM

"debug-info-no-location.cpp" is an extremely generic name for a very specific case. "debug-info-atexit-stub.cpp" would be better.

rnk added a comment.Aug 19 2019, 1:16 PM

I'd be happy to take this patch, address the comments, and land it, if you don't want to deal with all the nits.

test/CodeGen/debug-info-no-location.cpp
1 ↗(On Diff #215526)

I think the %clang_cc1 substitution is preferred, it sets the resource directory.

4 ↗(On Diff #215526)

I think using an artificial location (line 0) is consistent with other calls to StartFunction in CGDeclCXX.

aprantl added inline comments.Aug 19 2019, 2:42 PM
test/CodeGen/debug-info-no-location.cpp
4 ↗(On Diff #215526)

I was referring to the DIFlag::Artificial that translates into DISubprogram::isArtificial() here. The line 0 is orthogonal, but also nice to have!

aganea updated this revision to Diff 216046.Aug 19 2019, 7:46 PM
aganea marked 4 inline comments as done.

As requested.

probinson added inline comments.Aug 20 2019, 8:20 AM
test/CodeGenCXX/debug-info-atexit-stub.cpp
14

Do these Windows-mangled names work on Linux?

aganea updated this revision to Diff 216233.Aug 20 2019, 1:37 PM
aganea marked 2 inline comments as done.
aganea added inline comments.
test/CodeGenCXX/debug-info-atexit-stub.cpp
14

Ensure Microsoft mangling is always used.

aprantl accepted this revision.Aug 20 2019, 2:11 PM
This revision is now accepted and ready to land.Aug 20 2019, 2:11 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2019, 3:08 PM
aganea reopened this revision.Aug 26 2019, 7:43 PM
aganea added a reviewer: hans.

Re-opening this because the previous commit broke Chromium.

  • Added a new test debug-info-destroy-helper.cpp to cover the Chromium issue.
  • Also added CGDeclCXX.cpp, L775 to create an artificial location for __cxx_global_array_dtor.

Would you possibly please take another look?

This revision is now accepted and ready to land.Aug 26 2019, 7:43 PM
aganea requested review of this revision.Aug 26 2019, 7:45 PM
aganea updated this revision to Diff 217298.

I don't see a test for the __cxx_global_array_dtor case?

I don't see a test for the __cxx_global_array_dtor case?

It is actually the arraydestroy.* loop that is covered (generated by CodeGenFunction::emitArrayDestroy), please see debug-info-destroy-helper.cpp below.

I don't see a test for the __cxx_global_array_dtor case?

It is actually the arraydestroy.* loop that is covered (generated by CodeGenFunction::emitArrayDestroy), please see debug-info-destroy-helper.cpp below.

I was expecting a check for the Artificial flag, though.

aganea updated this revision to Diff 217968.Aug 29 2019, 1:56 PM
aganea retitled this revision from [DebugInfo] Add debug location to dynamic atexit destructor to [DebugInfo] Add debug location to stubs generated by CGDeclCXX and mark them as artificial.

More tagging functions as artificial.

rnk added inline comments.Aug 29 2019, 2:12 PM
lib/CodeGen/CGDebugInfo.cpp
3576

Is it OK to look up the lexical block stack at this point? The block stack isn't function local, it's part of CGDebugInfo, which is for the whole module, unlike CodeGenFunction. If we start emitting one of these thunks while we're emitting some other function, we could get some strange results. Does anything ensure we've pushed at least one scope by the time we come here?

aganea updated this revision to Diff 218515.Sep 3 2019, 12:25 PM
aganea marked 2 inline comments as done.
aganea added inline comments.
lib/CodeGen/CGDebugInfo.cpp
3576

You're right, it was looking too much like smart code. Replaced by a new flag DynamicInitKind::Global to make explicit the decision to emit an artificial function.

aganea marked an inline comment as done.Sep 3 2019, 12:27 PM
aganea added inline comments.
lib/CodeGen/CGDebugInfo.cpp
3576

Actually, it is DynamicInitKind::GlobalDestructor, sorry about that.

rnk added a comment.Sep 4 2019, 11:51 AM

I'm trying to figure out what exactly the new GlobalDestructor thing is. :) It's not clear to me why we ever emit __cxx_global_array_dtor in the MS ABI. We always call it directly from the ?__F atexit helper stub instead of registering it, so we really don't need it at all. It seems like something you'd only need for Itanium + -fno-use-cxa-atexit. Anyway, there's no reason we can't give it it's own kind, since we need to have it for Itanium anyway.

rnk accepted this revision.Sep 4 2019, 1:19 PM

lgtm with the GlobalArrayDestructor name.

include/clang/AST/GlobalDecl.h
34 ↗(On Diff #218515)

Maybe we should rename this GlobalArrayDestructor, since it's only used for arrays. Later, we can come back and avoid emitting these when using the MS ABI, since we already have the ?__F finalizer.

This revision is now accepted and ready to land.Sep 4 2019, 1:19 PM
This revision was automatically updated to reflect the committed changes.