Page MenuHomePhabricator

emit_DW_AT_noreturn flag
ClosedPublic

Authored by vleschuk on Aug 4 2016, 9:10 AM.

Details

Summary

Support for DW_AT_noreturn DWARF flag. Add it to debug info in case function is C++11 [[ noreturn ]] or C11 _Noreturn.

PS Corresponding clang patch is following

Diff Detail

Event Timeline

vleschuk updated this revision to Diff 66814.Aug 4 2016, 9:10 AM
vleschuk retitled this revision from to emit_DW_AT_noreturn flag.
vleschuk updated this object.
vleschuk added a reviewer: asl.
vleschuk added a subscriber: llvm-commits.
vleschuk updated this revision to Diff 66822.Aug 4 2016, 10:07 AM
vleschuk added reviewers: dexonsmith, echristo, aprantl.
vleschuk added a subscriber: cfe-commits.

Expanded context, added more reviewers who took part in this parts of code.

aprantl edited edge metadata.Aug 4 2016, 10:18 AM

Thanks for working in this!
Technically this is a DWARF 5 feature, but unknown attributes can be ignored by consumers, so there's no reason to emit it conditionally.

Could you please also add a test for llvm-dwarfdump?
Could you please also add a textual LLVM IR parser testcase?

include/llvm/IR/DebugInfoMetadata.h
1424

The \brief is redundant. We compile doxygen with autobrief now.
Also, there's a missing . at the end.

vleschuk updated this revision to Diff 67212.Aug 8 2016, 12:07 PM
vleschuk edited edge metadata.

Added C++11, C11 and ObjC textual llvm-dwarfdump tests.

Thanks, now all that's missing is an LLVM IR round-trip test. Adding the new flag to test/Assembler/disubprogram would work.

test/DebugInfo/noreturn_c11.ll
10

Maybe check for just a bit more context (the DW_TAG_subprogram + DW_AT_name should be fine)?

30

We typically strip out all unnecessary attributes to not distract from the testcase. A good heuristic is that you can usually get rid of everything in quotes.

test/DebugInfo/noreturn_cpp11.ll
10

More context.

37

Strip attributes.

test/DebugInfo/noreturn_objc.ll
11

more context

32

strip attributes

vleschuk updated this revision to Diff 67398.Aug 9 2016, 12:53 PM
vleschuk marked 6 inline comments as done.

Stripped unnecessary attributes, added more context to CHECK

aprantl accepted this revision.Aug 9 2016, 1:10 PM
aprantl edited edge metadata.

LGTM on the condition that the missing IR roundtrip test (e.g., by extending test/Assembler/disubprogram.ll) is added before committing.

This revision is now accepted and ready to land.Aug 9 2016, 1:10 PM

Found one more.

test/DebugInfo/noreturn_c11.ll
11

One more thing: please rewrite this (and the other testcases) as

; CHECK: DW_TAG_subprogram 
; CHECK-NOT: DW_TAG
; CHECK: DW_AT_name{{.*}}"f"
; CHECK-NOT: DW_TAG
; CHECK: DW_AT_noreturn
``

The check for the name ensures we're in the right function, the CHECK-NOT: ensures that we are not skipping into a subsequent tag.
vleschuk updated this revision to Diff 67410.Aug 9 2016, 1:44 PM
vleschuk edited edge metadata.

Added more tests

Found one more.

Done

include/llvm/IR/DebugInfoMetadata.h
1424

Just following the pattern in file. Added dot.

Could somebody commit this, please? I do not have the commit permissions yet.

Did you forget to add the IR test before uploading the patch?

vleschuk updated this revision to Diff 67462.Aug 9 2016, 10:16 PM

Stripped more unnecessary code. And cosmetics: trailing spaces.

Did you forget to add the IR test before uploading the patch?

Could you please clarify, what do you mean? All the IR tests are here now.

You are adding a new constant to the LLVM IR, so there should be a round-trip test that tests the LLVM IR parser, bitcode writer, bitcode reader, and LLVM IR printer to make sure we're handling the new constant correctly.

One easy way to do this is by adding the new flag to the already existing test in test/IR/disubprogram.ll.

vleschuk updated this revision to Diff 68327.Aug 17 2016, 3:15 AM
vleschuk marked an inline comment as done.

Added test for noreturn flag for test/Assembler/disubprogram.ll

vleschuk updated this revision to Diff 68328.Aug 17 2016, 3:17 AM

Full context

vleschuk updated this revision to Diff 68330.Aug 17 2016, 3:21 AM

Full context

vleschuk marked 2 inline comments as done.Aug 17 2016, 4:08 AM

Replied on doxygen-related comment: I do not think we should use different styles within one file.

Could somebody commit this, please?

aprantl closed this revision.Aug 17 2016, 9:11 AM

Committed as r278940. Thanks!