Page MenuHomePhabricator

[DebugInfoMetadata] Added support to generate packed_decimal encoding related dwarf info.
AbandonedPublic

Authored by Chirag on Sep 20 2018, 3:15 AM.

Details

Reviewers
echristo
aprantl
Summary

supports to mark DW_TAG_base_type with DW_AT_picture_string, DW_AT_digit_count, DW_AT_decimal_scale and DW_AT_decimal_sign.

Diff Detail

Event Timeline

Chirag created this revision.Sep 20 2018, 3:15 AM

Thanks!, Comments are inline. Please also update the metadata unit test, to test the stuff in LLVMContext.

include/llvm/BinaryFormat/Dwarf.def
707

Thanks! Looks like none of these were introduced recently so we can get away with not encoding the version number this was introduced in.

include/llvm/IR/DIBuilder.h
211

I think I would prefer a

struct DecimalInfo {
 unsigned DigitCount;
 unsigned DecimalSign;
 int DecimalScale;
};

DIBasicType *createBasicType(..., Optional<DecimalInfo>() = {}, ...

Since this is more of an all-or-nothing affair.

include/llvm/IR/DebugInfoMetadata.h
741

Perhaps you could reuse this struct for the optional arg I mentioned above?

742

Doxygen comments?

750

I don't think this is needed?

843

I think these assertions are redundant and can be removed, because the Optional deref operation would also assert.

lib/IR/DIBuilder.cpp
268

Same here, I think having an Optional<> for all the decimal info would be preferable. (Do all of these values even have a N/A-sentinel value otherwise?)

I will also include the metadata unit test with listed changes, thanks.

include/llvm/IR/DebugInfoMetadata.h
843

i will remove them, thanks.

lib/IR/DIBuilder.cpp
268

All those are optional and one or more may be absent for some encoding type, i will re-factor the code instead of all-or-nothing to all-optional.

After having one more look, currently scale is one which does not have invalid value to mark with, yes it would be better to use optional so all of them will have same N/A-sentinal values instead of different for each of them.

Thanks! , i will make the changes.

Chirag updated this revision to Diff 166857.Sep 25 2018, 5:37 AM
  • Updated metatdata unittest
  • Fixed optional issues with decimal attributes.
  • Added doxygen comments.
  • Updated asm roundtrip tests.
aprantl added inline comments.Sep 25 2018, 9:04 AM
include/llvm/IR/DIBuilder.h
211

Ok according to you comment later, these are all optional by themselves.

213

You will also need to provide a clang patch to use the new interface.

include/llvm/IR/DebugInfoMetadata.h
743

.

745

We don't typically combine Optional<> with pointer types but use the nullptr value as a sentinel instead.

746

Do you need the full 32 bits here, or could you pack this tighter?
With the optional this may use up to 64 bits...?

760

What's the point of having this Optional and all fields inside Optional, too?
If we want to save space, we could optionally allocate trailing memory, or created a derived class DIDecimalType with the extra fields.

lib/AsmParser/LLParser.cpp
4354

clang-format please

lib/Bitcode/Reader/MetadataLoader.cpp
1227

clang-format please

unittests/IR/MetadataTest.cpp
1149

It might be better to use a fully populated DecimalInfo here?

1160

The idea of these tests is that you change exactly one argument each time, here' you're changing the picture string, too, so all the other differences don't matter any more.

Chirag added inline comments.Sep 26 2018, 1:44 AM
include/llvm/IR/DIBuilder.h
213

correct me if i am wrong, clang do not support the languages which uses the related data type encoding, which uses this optional dwarf generation for base type.

include/llvm/IR/DebugInfoMetadata.h
745

i will use nullptr for N/A-sentinal value instead of None.

746

not really, i will try to decrease the memory footprint for this struct.

aprantl added inline comments.Sep 26 2018, 5:24 PM
include/llvm/IR/DIBuilder.h
213

I was blindly assuming that clang was setting the DIFlags argument. If it leaves it blank, then you don't need to do anything.

+llvm-commits

clang-format-diff is your friend.

Remind me of your use case? Clang does not support any decimal types, although there is work in progress to support scaled binary.

include/llvm/BinaryFormat/Dwarf.def
702

Need a space after the //

707

Right the decimal stuff was all added in DWARF 3 and I'm pretty sure has not changed since. As these are enums used in an attribute that is correctly identified as v3, I don't think these need a version.

include/llvm/IR/DIBuilder.h
203

I don't know that DW_ATE_float is an appropriate example here.

204

Does it have to be "packed" decimal type? That may be your use case but as you are passing the encoding as a parameter, I think this would work for all the decimal types.

206

DecimalScale

lib/AsmParser/LLParser.cpp
4335

I'd prefer the example to be self-consistent (digits/scale matching the pic string).

lib/Bitcode/Writer/BitcodeWriter.cpp
1500

LLVM style uses full sentences in comments (with proper capitalization and punctuation).

lib/CodeGen/AsmPrinter/DwarfUnit.cpp
753

Space before the =.

@aprantl yes, at the moment clang is not generating bi-endian code, hence not using DIFlags.

@probinson, thanks i will fix format/typos related changes.
about the use cases, this patch will provide containers to convey and generate optional DWARF attributes for base type tag with below listed encodings,
DW_ATE_packed_decimal
DW_ATE_numeric_string
DW_ATE_edited
DW_ATE_signed_fixed
DW_ATE_unsigned_fixed

What I meant by "use case" is, do you have a front end that will make use of these new features?

Are you supporting binary scale as well as decimal scale, for [un]signed_fixed encodings? It looks like you are supporting only decimal scale.

What I meant by "use case" is, do you have a front end that will make use of these new features?

Are you supporting binary scale as well as decimal scale, for [un]signed_fixed encodings? It looks like you are supporting only decimal scale.

Apologies for late reply.
Yes, with this patch, only decimal scale is supported and i will add binary scale as well. (will modify this patch), and related changes too.
and about the front-end, we have a pl1/cobol llvm based front-end (in development) which generates this info.

Yes, with this patch, only decimal scale is supported and i will add binary scale as well. (will modify this patch), and related changes too.
and about the front-end, we have a pl1/cobol llvm based front-end (in development) which generates this info.

Awesome; is this the OpenVMS port or something else? I had thought there were maybe two people in the world with both COBOL and LLVM experience.
COBOL doesn't need binary scale but I believe PL/I does (not that I've used PL/I in the past 25 years). You'll want to think about whether it's simpler to use a single API for any scaled type, or split out decimal from binary. Binary scaled obviously doesn't need most of the new parameters.

Chirag updated this revision to Diff 168783.Oct 9 2018, 5:38 AM
  • Added DW_AT_binary_scale attribute.
  • Fixed llvm type code formatting
  • Fixed minor typos.

@probinson - no, it is not openvms port, instead we have our own PLI/Cobol compiler for which we are adding debugging support.

@aprantl - apartt from saving memory, here optional in optional is used to mark if basictype is extended or not, so instead of checking all underlying optional attributes only one check is needed.

Chirag updated this revision to Diff 168796.Oct 9 2018, 7:28 AM
  • clang-format
aprantl added inline comments.Oct 19 2018, 9:36 AM
include/llvm/IR/DebugInfoFlags.def
54 ↗(On Diff #168796)

The space in DIFLags is precious. Can you explain why we need this as a generic DIFlag and not just a bit in DIBasicType?

Chirag added inline comments.Oct 19 2018, 10:49 AM
include/llvm/IR/DebugInfoFlags.def
54 ↗(On Diff #168796)

Thanks for reply. i am thinking this is a bad design as data seems scattered and basicType is getting bloated also you are right about DIFlags' bit being reserved and used only in rare cases is not good.
i am thinking of adding new DIDecimalType class as you suggested earlier in DebugInfoMetaData.h inherited from Type and scrap this review if it is okay with you guys.

Chirag abandoned this revision.Oct 30 2018, 6:56 AM