Page MenuHomePhabricator

[DebugInfoMetadata] Added DIFlags interface in DIBasicType.
ClosedPublic

Authored by Chirag on Jul 20 2018, 11:17 AM.

Details

Summary

This change is only initial implementation. will be extended in Asm/Bitcode writer/reader and DIBuilder.
Flags in DIBasicType will be used to pass attributes used in DW_TAG_base_type, such as DW_AT_endianity.

Diff Detail

Repository
rL LLVM

Event Timeline

Chirag created this revision.Jul 20 2018, 11:17 AM

Testing Done: Compilation and llvm-check.

May I ask what the context of this is? Do you have a machine where some (but not all) basic types have a different endianity than the one specified by the ABI?

Hello Adrian,

Thanks for the reply.
Apologies for late answer.

I am currently working on supporting the gcc specific bi-endian syntactic and semantic convention attribute((bigendian)) for source level debugging for structures with basic type.
I searched for the extension and found out that endianity is completely missing from llvm debug info generation.
So i started from basicType.

I am planning to add this to Variable with Unspecified types, and formal parameters etc as described in DWARFv4.

Regards,
Chirag Patel.

Chirag updated this revision to Diff 158480.Aug 1 2018, 2:04 AM

Extended DIFlags to support DW_AT_endianity in basictype as per DWARFv4

Chirag added a comment.Aug 1 2018, 2:06 AM

For testing i have ran llvm-check with no regressions.

Do you need to support endinaness attributes just on basic types or eventually on any type?
This is still missing an assembler round-trip test.

include/llvm/IR/DebugInfoFlags.def
57 ↗(On Diff #158480)

Can you add context to the patch? (git diff -u 9999)
What's bit 1 used for normally?

Chirag added a comment.Aug 1 2018, 9:24 AM

Do you need to support endinaness attributes just on basic types or eventually on any type?

for the time being the basicType that maps to DW_TAG_base_type, but in future the variables created using that basictype or any other type with DW_TAG_variable and DW_TAG_formal_parameters tags.

This is still missing an assembler round-trip test.

I will test and update it.

Can you add context to the patch? (git diff -u 9999)

I will update it with missing round-trip test.

What's bit 1 used for normally?

Those lower bits are mapped to DW_END_default/big/little enums(that mapping is not used anywhere at the moment but as it was tristate i needed to use it so, ), and bit 0 is mapped to DW_END_default which does not need to be explicitly used if that flag is equal to absence of DW_AT_endianity as it is optional., and also i will remove the unnecessary shifts.

for the time being the basicType that maps to DW_TAG_base_type, but in future the variables created using that basictype or any other type with DW_TAG_variable and DW_TAG_formal_parameters tags.

Is it a property of the type or a property of the variable? I.e.: Can you have a struct with one LE member and one BE member?

Chirag added a comment.EditedAug 1 2018, 9:42 PM

for the time being the basicType that maps to DW_TAG_base_type, but in future the variables created using that basictype or any other type with DW_TAG_variable and DW_TAG_formal_parameters tags.

Is it a property of the type or a property of the variable? I.e.: Can you have a struct with one LE member and one BE member?

As far as dwarf standard is concerned it is both(type and variable),
case 1: when struct with 10 ints so, to add dwarf info we can create base_type with BE and reuse that type while creating 10 member variables.
case 2: when struct with 10 ints and void*(unspecified or incomplete type), to create for 10 ints use prev methos and for void* create variable with unspecified type and mark "variable" as BE.

i am aware that creating variable with base type and having endianity in both(type and variabel), is redundant, but as far as dwarf standard lists, the DW_AT_endianity attribute is used to mark both type and variables.

Can you have a struct with one LE member and one BE member?

as far as gcc goes no. but dwarf supports it by marking variables instead of type.

Chirag updated this revision to Diff 158692.Aug 2 2018, 1:00 AM
  • Included Full context.
  • Changed flag values for BigEndian, LittleEndian.
  • Included assembler round-trip test for BasicType with BigEndian/LittleEndian testcases.
aprantl added inline comments.Aug 2 2018, 7:30 AM
include/llvm/IR/DIBuilder.h
195 ↗(On Diff #158692)

We should not pass in raw Flags here. They are an implementation detail. Please use an enum { DW_END_default / DW_END_big / DW_END_little } instead. (It also looks like this enum is currently missing in Dwarf.def.)

test/Assembler/debug-info.ll
101 ↗(On Diff #158692)

Thanks, this is good.

aprantl added inline comments.Aug 2 2018, 7:33 AM
include/llvm/IR/DebugInfoFlags.def
57 ↗(On Diff #158480)

I like this more. There should also be a Verifier check that ensures that 27 & 28 cannot be set at the same time.

Chirag added inline comments.Aug 2 2018, 8:59 AM
include/llvm/IR/DIBuilder.h
195 ↗(On Diff #158692)

About rawflag, I was planning to use the same implementation as used in "createAutoVariable", which uses DIFlags, so in future the DW_AT_endianity can be extended to the same without much of a changes to mark the TAG_variables.

about DW_END_default/big/little are defined in dwarf.h and not used much anywhere else, so if we can work with rawflags then those need not be moved to dwarf.def for the moment being.

if you want it then i am happy to make those changes.

aprantl added inline comments.Aug 2 2018, 9:09 AM
include/llvm/IR/DIBuilder.h
195 ↗(On Diff #158692)

If there is precedence for passing in raw flags in DIBuilder then that's fine with me, too.

We will need the enum in Dwarf.def for llvm-dwarfdump anyway, so we might as well add it now.

lib/IR/LLVMContextImpl.h
399 ↗(On Diff #158692)

hashing the flags is going to be a waste of time on the vast majority of platforms. I would just leave this as is.

The last thing that is missing is a bitcode upgrade test (unless you can show that one of the existing checked in .bc files in the testsuite also covers the upgrade path for the old, shorter DIBasicType).

Chirag updated this revision to Diff 158939.Aug 3 2018, 1:00 AM
  • moved endianity enums to dwarf.def
  • Added verifier check for DIBasicType for invalid flags.
  • removed hashing of flags from DIBasicType
Chirag marked 3 inline comments as done.Aug 3 2018, 1:04 AM

The last thing that is missing is a bitcode upgrade test (unless you can show that one of the existing checked in .bc files in the testsuite also covers the upgrade path for the old, shorter DIBasicType).

It seems like there is no such dedicated test case for DIBasicType, but some other testcases such as DIVariable, DiExpression uses the DIBasicType.
Please advice, if you want me to create a new test case for bitcode upgrade of DIBasicType or is it okey to modify the existing DiVariable one.

It would be cleanest to create a new, minimal test that only contains one global variable with a basic type. The resulting bitcode should only be a few bytes and then it would be obvious what the tests tests.

Chirag updated this revision to Diff 159263.Aug 6 2018, 2:53 AM
  • Minor changes relating to DIFlags checks.
  • Added bitcode upgrade test for DIBasicType with flags endianity.
Chirag updated this revision to Diff 159265.Aug 6 2018, 3:01 AM
aprantl added inline comments.Aug 6 2018, 9:00 AM
lib/IR/Verifier.cpp
886 ↗(On Diff #159265)

same for this function name.

900 ↗(On Diff #159265)

FlagLittleEndian is not a "reference" flag. Either just say "has conflicting flags" or have two assertions one for reference and one for endianity.

unittests/IR/MetadataTest.cpp
1022 ↗(On Diff #159265)

You need to add one extra case:

EXPECT_NE(N, DIBasicType::get(Context, dwarf::DW_TAG_base_type, "special", 33,
                                26, 7, DINode::FlagEndianLittle));
Chirag added inline comments.Aug 7 2018, 3:52 AM
unittests/IR/MetadataTest.cpp
1022 ↗(On Diff #159265)

During one of our last review, we made BasicType node unique without using the flags, (as those are optional),

I have few concerns,
as DINode:FlagEndianLittle (no such flag present) it will fail at compile-time.

and if thats a typo and you mean (DiNode::FlagLittleEndian)
as we made node unique without considering flags (as per dwarf standard which is optional), it will be same as "N" (in our case), which will fail at runtime (we are trying EXPECT_NE),

adding this case does not make any sense.
Please advise more on the comment.

aprantl added inline comments.Aug 7 2018, 10:44 AM
unittests/IR/MetadataTest.cpp
1022 ↗(On Diff #159265)

The idea was to only remove the *hashing* of the flags (for performance reasons in the common case), but not the comparison. If two basic types hash the same, they would still fail the operator= check. So this test should work.

Chirag added inline comments.Aug 7 2018, 11:22 AM
unittests/IR/MetadataTest.cpp
1022 ↗(On Diff #159265)

Apologies in advance if my understanding is wrong,

During creation of basictype we check that if it is unique or not using hashing so, if we exclude flags we will have the same node, so operator= will succed.

aprantl added inline comments.Aug 7 2018, 12:45 PM
unittests/IR/MetadataTest.cpp
1022 ↗(On Diff #159265)

The hash function is just the first stage of the DenseMap lookup, if the hash function is equivalent then the comparison function is invoked. This is necessary because two hashes could conflict.

MDNodeKeyImpl<DIBasicType>::isKeyOf() is still comparing the Flags in the second stage of the lookup. (If it doesn't then we'll need to add this in this patch).

Chirag added inline comments.Aug 9 2018, 1:16 AM
unittests/IR/MetadataTest.cpp
1022 ↗(On Diff #159265)

Thanks for clarification. i will update the patch soon.

Chirag updated this revision to Diff 159870.EditedAug 9 2018, 1:21 AM
  • fixed unique node lookup for basicType
  • fixed assert message for conflicting flags.
  • added testcases for comparing basictype with flags.
Chirag marked 12 inline comments as done.Aug 9 2018, 10:17 PM
aprantl added inline comments.Aug 10 2018, 4:05 PM
lib/IR/LLVMContextImpl.h
394 ↗(On Diff #159870)

thanks!

test/Bitcode/DIBasicType-DIFlags.ll
1 ↗(On Diff #159870)

This test is supposed to test that the *old* format (without the new Flags field) can be parsed. There is no need to check in a .bc file with the new format. That case is already covered by the round-trip tests.

27 ↗(On Diff #159870)

Please use just one DIBasicType without any flags, as generated by current LLVM trunk.

Chirag added inline comments.Aug 11 2018, 7:45 AM
test/Bitcode/DIBasicType-DIFlags.ll
1 ↗(On Diff #159870)

Oh, my bad,
In that case there are plenty of testcases that checks just same, covering many off the DIBasicType old format,
so i will remove this testcase as it is not required.

few examples:
test/Bitcode/DIExpression-minus-upgrade.ll
test/Bitcode/disubrange.ll
test/Bitcode/disubrange.ll
test/Bitcode/dilocalvariable-3.9.ll
test/Bitcode/dilocalvariable-3.9.ll
test/Bitcode/disubrange-v0.ll
test/Bitcode/disubrange-v0.ll
test/Bitcode/DIExpression-deref.ll
test/Bitcode/DIExpression-aggresult.ll
test/Bitcode/DIGlobalVariableExpression.ll
test/Bitcode/DIGlobalVariableExpression2.ll
test/Bitcode/upgrade-dbg-value.ll
test/Bitcode/diglobalvariable-3.8.ll
test/Bitcode/DIExpression-4.0.ll
test/Bitcode/DINamespace.ll

Thanks.

Chirag updated this revision to Diff 160308.Aug 13 2018, 1:43 AM
  • removed wrong testcase.
aprantl accepted this revision.Aug 13 2018, 9:27 AM

Thanks!

This revision is now accepted and ready to land.Aug 13 2018, 9:27 AM
Chirag marked an inline comment as done.Aug 13 2018, 9:50 AM

Hello adrian,

Thanks.
Can you kindly push this changes, as i do not have write permissions on repo.

Regards,
Chirag Patel.

This revision was automatically updated to reflect the committed changes.