Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

[DebugInfo] Expose Fortran array debug info attributes through DIBuilder.
ClosedPublic

Authored by cchen15 on Oct 20 2020, 12:40 PM.

Details

Summary

The support of a few debug info attributes specifically for Fortran arrays have been added to LLVM recently, but there's no way to take advantage of them through DIBuilder. This patch extends DIBuilder::createArrayType to enable the settings of those attributes.

Diff Detail

Event Timeline

cchen15 created this revision.Oct 20 2020, 12:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 20 2020, 12:40 PM
cchen15 requested review of this revision.Oct 20 2020, 12:40 PM

I'd appreciate some guidance on what test to add for this change. Would it be sufficient to have a unittest that calls createArrayType to create a DICompositeType, and then get the three new attributes back from the DICompositeType using the getRaw* functions to check that they match with the arguments to the createArrayType call? Thanks.

Can you also add a unit test for this?

llvm/include/llvm/IR/DIBuilder.h
497

nit: . at the end please

498

Please also document what kind of DINode is expected here.

498

Can these be any more specific DINode types?

Regarding test: Does the test idea laid out in my opening comment sound reasonable?

llvm/include/llvm/IR/DIBuilder.h
498

These can be DIExpression, ConstantAsMetadata, etc, so Metadata looks to be the right specificity (and the underlying 'get' function it calls types them as Metadata too).

cchen15 updated this revision to Diff 299655.Oct 21 2020, 5:59 AM
cchen15 marked 2 inline comments as done.

Per review comments, fixed/enhanced the description of createArrayType.

cchen15 updated this revision to Diff 299656.Oct 21 2020, 6:03 AM

Per review comments, fixed/enhanced the description of createTypeArray.

Sorry. Please ignore Patch 2.

cchen15 updated this revision to Diff 299702.Oct 21 2020, 8:29 AM

Added a unit test for the patch.

alok added inline comments.Oct 21 2020, 8:43 AM
llvm/lib/IR/DIBuilder.cpp
530

You may want to add "Rank" as well. (D89141)

cchen15 updated this revision to Diff 299718.Oct 21 2020, 9:19 AM

Including the rank attribute in the patch.

cchen15 marked an inline comment as done.Oct 21 2020, 9:20 AM

Thanks for the patch and adding the unit-test for the same. This API is still un-used(directly) (classic flang uses it's own way to construct this.).
The new Flang is the planned consumer for it, Although since most CodeGen part of new Flang is still lives downstream. There's not much we can do about it WRT to debug-info and these API utilization.

LGTM! I'll leave to @aprantl for official approval(since he's senior member and understands the API interfacing and debug metadata much better) :).

These can be DIExpression, ConstantAsMetadata, etc, so Metadata looks to be the right specificity (and the underlying 'get' function it calls types them as Metadata too).

Why do we need to accept both DIExpression and ConstantAsMetadata? Since we can also represent all constants as DIExpression, I think I would prefer to have only one way to represent a constant. Would it be feasible to expect clients to pass constants as DIExpression(DW_OP_constu, <number>, DW_OP_stack_value)? And if it doesn't already exist perhaps add a DIExpression *DIBuilder::createConstantExpression(unsigned value) API?

These can be DIExpression, ConstantAsMetadata, etc, so Metadata looks to be the right specificity (and the underlying 'get' function it calls types them as Metadata too).

Why do we need to accept both DIExpression and ConstantAsMetadata? Since we can also represent all constants as DIExpression, I think I would prefer to have only one way to represent a constant. Would it be feasible to expect clients to pass constants as DIExpression(DW_OP_constu, <number>, DW_OP_stack_value)? And if it doesn't already exist perhaps add a DIExpression *DIBuilder::createConstantExpression(unsigned value) API?

We can do that, but then the debugger would need to work a bit harder in order to get a constant, doesn't it? I don't feel strongly either way. If you do want it done the way you prefer, please let me know and I will make the change.

cchen15 updated this revision to Diff 300093.Oct 22 2020, 1:39 PM

Typed the attributes as DIExpression*, as suggested by @aprantl.

Also, DIBuilder::createConstantValueExpression(uint64_t) is already available for doing constant via DIExpression,, and its usage is demonstrated in the unit test.

alok added a comment.Oct 23 2020, 12:52 AM

Typed the attributes as DIExpression*, as suggested by @aprantl.

Sorry for late comment. But We would miss the DIVariable type in that case which is also permissible for attributes, which later (in DWARF) is converted to DIE reference.

cchen15 updated this revision to Diff 300217.Oct 23 2020, 4:04 AM

Reverted back to type those attributes as Metadata to allow DIVairbale.

Thanks, @alok, for pointing out the DIVariable case.

We can do that, but then the debugger would need to work a bit harder in order to get a constant, doesn't it? I don't feel strongly either way. If you do want it done the way you prefer, please let me know and I will make the change.

That's an interesting point. Do we emit different DWARF FORMs for constant vs constant DIExpression? An elegant solution would be to recognize constant DIExpressions in AsmPrinter (DIExpression::isConstant) and emit the same DWARF FORM we would emit for an IR constant if the expression is a constant.

aprantl added inline comments.Oct 26 2020, 9:31 AM
llvm/include/llvm/IR/DIBuilder.h
508

We could make this still strongly typed by either having two variants of the function or by using a PointerUnion<DIExpression, DIVariable> here.

We can do that, but then the debugger would need to work a bit harder in order to get a constant, doesn't it? I don't feel strongly either way. If you do want it done the way you prefer, please let me know and I will make the change.

That's an interesting point. Do we emit different DWARF FORMs for constant vs constant DIExpression? An elegant solution would be to recognize constant DIExpressions in AsmPrinter (DIExpression::isConstant) and emit the same DWARF FORM we would emit for an IR constant if the expression is a constant.

This optimization is orthogonal to the change proposed here, IMHO. I'd prefer keep this patch to DIBuilder, and do that optimization in a separate patch. Would that be ok with you?

cchen15 updated this revision to Diff 300769.Oct 26 2020, 1:02 PM

Typed the attributes as PointerUnion<DIExpression *, DIVariable *>, and updated the test to exercise the DIVariable case.

cchen15 marked an inline comment as done.Oct 26 2020, 1:05 PM
cchen15 added inline comments.
llvm/include/llvm/IR/DIBuilder.h
508

Thanks for pointing this out.

aprantl accepted this revision.Oct 26 2020, 2:35 PM
This revision is now accepted and ready to land.Oct 26 2020, 2:35 PM
hctim added a subscriber: hctim.Oct 27 2020, 9:08 PM

This patch unfortunately caused a regression in the sanitizer buildbots, namely a check-llvm regression under ASan. Please take a look:

http://lab.llvm.org:8011/#/builders/99/builds/253

********************
Testing:  0..
FAIL: LLVM-Unit :: IR/./IRTests/DIBuilder.CreateFortranArrayTypeWithAttributes (3304 of 39961)
******************** TEST 'LLVM-Unit :: IR/./IRTests/DIBuilder.CreateFortranArrayTypeWithAttributes' FAILED ********************
Note: Google Test filter = DIBuilder.CreateFortranArrayTypeWithAttributes
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from DIBuilder
[ RUN      ] DIBuilder.CreateFortranArrayTypeWithAttributes
[       OK ] DIBuilder.CreateFortranArrayTypeWithAttributes (1 ms)
[----------] 1 test from DIBuilder (1 ms total)
[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (2 ms total)
[  PASSED  ] 1 test.
=================================================================
==1460==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 104 byte(s) in 1 object(s) allocated from:
    #0 0x6975b8 in operator new(unsigned long) /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/compiler-rt/lib/asan/asan_new_delete.cpp:99:3
    #1 0x1990b21 in llvm::MDNode::operator new(unsigned long, unsigned int) /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/lib/IR/Metadata.cpp:485:40
    #2 0x17b3aa3 in llvm::DIGlobalVariable::getImpl(llvm::LLVMContext&, llvm::Metadata*, llvm::MDString*, llvm::MDString*, llvm::Metadata*, unsigned int, llvm::Metadata*, bool, bool, llvm::Metadata*, llvm::Metadata*, unsigned int, llvm::Metadata::StorageType, bool) /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/lib/IR/DebugInfoMetadata.cpp:902:3
    #3 0x17736e8 in getImpl /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/include/llvm/IR/DebugInfoMetadata.h:2827:12
    #4 0x17736e8 in getTemporary /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/include/llvm/IR/DebugInfoMetadata.h:2848:3
    #5 0x17736e8 in llvm::DIBuilder::createTempGlobalVariableFwdDecl(llvm::DIScope*, llvm::StringRef, llvm::StringRef, llvm::DIFile*, unsigned int, llvm::DIType*, bool, llvm::MDNode*, llvm::MDTuple*, unsigned int) /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/lib/IR/DIBuilder.cpp:697:10
    #6 0x8723f4 in (anonymous namespace)::DIBuilder_CreateFortranArrayTypeWithAttributes_Test::TestBody() /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/unittests/IR/DebugInfoTest.cpp:213:11
    #7 0x256d6c0 in HandleExceptionsInMethodIfSupported<testing::Test, void> /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc
    #8 0x256d6c0 in testing::Test::Run() /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2474:5
    #9 0x256fcd5 in testing::TestInfo::Run() /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2656:11
    #10 0x25710d7 in testing::TestCase::Run() /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2774:28
    #11 0x258f30d in testing::internal::UnitTestImpl::RunAllTests() /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:4649:43
    #12 0x258e5c0 in HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc
    #13 0x258e5c0 in testing::UnitTest::Run() /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:4257:10
    #14 0x25530e0 in RUN_ALL_TESTS /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest.h:2233:46
    #15 0x25530e0 in main /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/utils/unittest/UnitTestMain/TestMain.cpp:50:10
    #16 0x7f9c9071209a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)
Indirect leak of 128 byte(s) in 1 object(s) allocated from:
    #0 0x6975b8 in operator new(unsigned long) /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/compiler-rt/lib/asan/asan_new_delete.cpp:99:3
    #1 0x198df3e in make_unique<llvm::ReplaceableMetadataImpl, llvm::LLVMContext &> /b/sanitizer-x86_64-linux-bootstrap/build/libcxx_build_asan/include/c++/v1/memory:2816:28
    #2 0x198df3e in llvm::ContextAndReplaceableUses::getOrCreateReplaceableUses() /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/include/llvm/IR/Metadata.h:816:23
    #3 0x198a533 in getOrCreate /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/lib/IR/Metadata.cpp:315:51
    #4 0x198a533 in llvm::MetadataTracking::track(void*, llvm::Metadata&, llvm::PointerUnion<llvm::MetadataAsValue*, llvm::Metadata*>) /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/lib/IR/Metadata.cpp:157:17
    #5 0x19955d3 in track /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/include/llvm/IR/Metadata.h
    #6 0x19955d3 in llvm::MDOperand::reset(llvm::Metadata*, llvm::Metadata*) /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/include/llvm/IR/Metadata.h:740:5
    #7 0x19910f2 in setOperand /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/lib/IR/Metadata.cpp:874:22
    #8 0x19910f2 in llvm::MDNode::MDNode(llvm::LLVMContext&, unsigned int, llvm::Metadata::StorageType, llvm::ArrayRef<llvm::Metadata*>, llvm::ArrayRef<llvm::Metadata*>) /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/lib/IR/Metadata.cpp:511:5
    #9 0x17a8fe5 in DINode /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/include/llvm/IR/DebugInfoMetadata.h:125:9
    #10 0x17a8fe5 in DIScope /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/include/llvm/IR/DebugInfoMetadata.h:426:9
    #11 0x17a8fe5 in DIType /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/include/llvm/IR/DebugInfoMetadata.h:619:9
    #12 0x17a8fe5 in DICompositeType /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/include/llvm/IR/DebugInfoMetadata.h:1007:9
    #13 0x17a8fe5 in llvm::DICompositeType::getImpl(llvm::LLVMContext&, unsigned int, llvm::MDString*, llvm::Metadata*, unsigned int, llvm::Metadata*, llvm::Metadata*, unsigned long, unsigned int, unsigned long, llvm::DINode::DIFlags, llvm::Metadata*, unsigned int, llvm::Metadata*, llvm::Metadata*, llvm::MDString*, llvm::Metadata*, llvm::Metadata*, llvm::Metadata*, llvm::Metadata*, llvm::Metadata*, llvm::Metadata::StorageType, bool) /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/lib/IR/DebugInfoMetadata.cpp:524:3
    #14 0x177290b in getImpl /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/include/llvm/IR/DebugInfoMetadata.h:1031:12
    #15 0x177290b in get /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/include/llvm/IR/DebugInfoMetadata.h:1059:3
    #16 0x177290b in llvm::DIBuilder::createArrayType(unsigned long, unsigned int, llvm::DIType*, llvm::MDTupleTypedArrayWrapper<llvm::DINode>, llvm::PointerUnion<llvm::DIExpression*, llvm::DIVariable*>, llvm::PointerUnion<llvm::DIExpression*, llvm::DIVariable*>, llvm::PointerUnion<llvm::DIExpression*, llvm::DIVariable*>, llvm::PointerUnion<llvm::DIExpression*, llvm::DIVariable*>) /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/lib/IR/DIBuilder.cpp:534:13
    #17 0x872979 in (anonymous namespace)::DIBuilder_CreateFortranArrayTypeWithAttributes_Test::TestBody() /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/unittests/IR/DebugInfoTest.cpp:218:36
    #18 0x256d6c0 in HandleExceptionsInMethodIfSupported<testing::Test, void> /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc
    #19 0x256d6c0 in testing::Test::Run() /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2474:5
    #20 0x256fcd5 in testing::TestInfo::Run() /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2656:11
    #21 0x25710d7 in testing::TestCase::Run() /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2774:28
    #22 0x258f30d in testing::internal::UnitTestImpl::RunAllTests() /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:4649:43
    #23 0x258e5c0 in HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc
    #24 0x258e5c0 in testing::UnitTest::Run() /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:4257:10
    #25 0x25530e0 in RUN_ALL_TESTS /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest.h:2233:46
    #26 0x25530e0 in main /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/utils/unittest/UnitTestMain/TestMain.cpp:50:10
    #27 0x7f9c9071209a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)
SUMMARY: AddressSanitizer: 232 byte(s) leaked in 2 allocation(s).

********************
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 
********************
Failed Tests (3):
  ...
  LLVM-Unit :: IR/./IRTests/DIBuilder.CreateFortranArrayTypeWithAttributes

Note that there are two other tests that failed previously, from https://reviews.llvm.org/D89486#2358100.