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.
Details
Diff Detail
Event Timeline
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.
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 | Can you add context to the patch? (git diff -u 9999) |
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?
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.
- Included Full context.
- Changed flag values for BigEndian, LittleEndian.
- Included assembler round-trip test for BasicType with BigEndian/LittleEndian testcases.
include/llvm/IR/DebugInfoFlags.def | ||
---|---|---|
57 | I like this more. There should also be a Verifier check that ensures that 27 & 28 cannot be set at the same time. |
include/llvm/IR/DIBuilder.h | ||
---|---|---|
195 | 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. |
include/llvm/IR/DIBuilder.h | ||
---|---|---|
195 | 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).
- moved endianity enums to dwarf.def
- Added verifier check for DIBasicType for invalid flags.
- removed hashing of flags from DIBasicType
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.
- Minor changes relating to DIFlags checks.
- Added bitcode upgrade test for DIBasicType with flags endianity.
lib/IR/Verifier.cpp | ||
---|---|---|
886 | same for this function name. | |
900 | 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 | You need to add one extra case: EXPECT_NE(N, DIBasicType::get(Context, dwarf::DW_TAG_base_type, "special", 33, 26, 7, DINode::FlagEndianLittle)); |
unittests/IR/MetadataTest.cpp | ||
---|---|---|
1022 | During one of our last review, we made BasicType node unique without using the flags, (as those are optional), I have few concerns, and if thats a typo and you mean (DiNode::FlagLittleEndian) adding this case does not make any sense. |
unittests/IR/MetadataTest.cpp | ||
---|---|---|
1022 | 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. |
unittests/IR/MetadataTest.cpp | ||
---|---|---|
1022 | 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. |
unittests/IR/MetadataTest.cpp | ||
---|---|---|
1022 | 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). |
unittests/IR/MetadataTest.cpp | ||
---|---|---|
1022 | Thanks for clarification. i will update the patch soon. |
- fixed unique node lookup for basicType
- fixed assert message for conflicting flags.
- added testcases for comparing basictype with flags.
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. |
test/Bitcode/DIBasicType-DIFlags.ll | ||
---|---|---|
1 ↗ | (On Diff #159870) | Oh, my bad, few examples: Thanks. |
Hello adrian,
Thanks.
Can you kindly push this changes, as i do not have write permissions on repo.
Regards,
Chirag Patel.
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.)