DebugInfo: purpose of align field

Hello all,

I am currently implementing support for DWARFv5 DW_AT_alignment attr and I got a question about align field in debug info section of IR/Bitcode. Currently it is being dumped almost in any case, however according to code we use align from DI* objects only when dealing with class/structure bitfields: DwarfUnit::constructMemberDIE.

Dumping align information everywhere only for 1 case looks like overhead to me.

Consider the following code:

struct S {
   char c;
} s;

When compiled with debug enabled in IR we get smth like that:

!6 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "S", file: !5, line: 1, size: 8, align: 8, elements: !7)

Am I missing something? What is the purpose of "align: 8" here? Maybe we could include alignment information into DI* objects only when required (and thus dump it only when required): for types with bitfields and when alignas() was specified in code.

Please advise.

We don’t really need to emit alignment information for bitfields anymore, we emit enough information in the MD to exactly describe the offset of the storage unit the bitfield is within (DIFlagBitField implies that the extraData field encodes this information).

Hello all,

I am currently implementing support for DWARFv5 DW_AT_alignment attr and I got a question about align field in debug info section of IR/Bitcode.

Thanks for looking into this!

Currently it is being dumped almost in any case, however according to code we use align from DI* objects only when dealing with class/structure bitfields: DwarfUnit::constructMemberDIE.

Dumping align information everywhere only for 1 case looks like overhead to me.

One your patch is finished it will be used in more cases, right? :slight_smile:

Consider the following code:

struct S {
char c;
} s;

When compiled with debug enabled in IR we get smth like that:

!6 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "S", file: !5, line: 1, size: 8, align: 8, elements: !7)

Am I missing something? What is the purpose of "align: 8" here? Maybe we could include alignment information into DI* objects only when required (and thus dump it only when required): for types with bitfields and when alignas() was specified in code.

What do you mean by "include" in this context? Are you arguing for making an alignment of 8 the implicit default in textual IR? Or do you want to make DICompositeType variable length?

-- adrian

Hello Adrian, sorry for the delay with the response. Somehow I missed your message..

Hello all,

I am currently implementing support for DWARFv5 DW_AT_alignment attr and I got a question about align field in debug info section of IR/Bitcode.

Thanks for looking into this!

No problem =)

Currently it is being dumped almost in any case, however according to code we use align from DI* objects only when dealing with class/structure bitfields: DwarfUnit::constructMemberDIE.

Dumping align information everywhere only for 1 case looks like overhead to me.

One your patch is finished it will be used in more cases, right? :slight_smile:

My point is that we dump align information everywhere and it is currently used only in 1 case. I am trying to make it so that it is dumped only when it is necessary.

Consider the following code:

struct S {
  char c;
} s;

When compiled with debug enabled in IR we get smth like that:

!6 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "S", file: !5, line: 1, size: 8, align: 8, elements: !7)

Am I missing something? What is the purpose of "align: 8" here? Maybe we could include alignment information into DI* objects only when required (and thus dump it only when required): for types with bitfields and when alignas() was specified in code.

What do you mean by "include" in this context? Are you arguing for making an alignment of 8 the implicit default in textual IR? Or do you want to make DICompositeType variable length?

I mean that in this case alignment isn't used anywhere in code further. I think we don't need alignment information in textual DI IR in case it is not default. Dumping default alignment in every case doesn't make sense for me: if we dump align info each time we need to mark "special" cases with additional flag in order to be able to know which case to "report" in DWARF. If we dump only "forced" alignment, we can just say: "Ok, alignment is specified, we need to put this into DWARF".

Hello Adrian, sorry for the delay with the response. Somehow I missed your message..

Hello all,

I am currently implementing support for DWARFv5 DW_AT_alignment attr and I got a question about align field in debug info section of IR/Bitcode.

Thanks for looking into this!

No problem =)

Currently it is being dumped almost in any case, however according to code we use align from DI* objects only when dealing with class/structure bitfields: DwarfUnit::constructMemberDIE.

Dumping align information everywhere only for 1 case looks like overhead to me.

One your patch is finished it will be used in more cases, right? :slight_smile:

My point is that we dump align information everywhere and it is currently used only in 1 case. I am trying to make it so that it is dumped only when it is necessary.

Consider the following code:

struct S {
char c;
} s;

When compiled with debug enabled in IR we get smth like that:

!6 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "S", file: !5, line: 1, size: 8, align: 8, elements: !7)

Am I missing something? What is the purpose of "align: 8" here? Maybe we could include alignment information into DI* objects only when required (and thus dump it only when required): for types with bitfields and when alignas() was specified in code.

What do you mean by "include" in this context? Are you arguing for making an alignment of 8 the implicit default in textual IR? Or do you want to make DICompositeType variable length?

I mean that in this case alignment isn't used anywhere in code further. I think we don't need alignment information in textual DI IR in case it is not default. Dumping default alignment in every case doesn't make sense for me: if we dump align info each time we need to mark "special" cases with additional flag in order to be able to know which case to "report" in DWARF. If we dump only "forced" alignment, we can just say: "Ok, alignment is specified, we need to put this into DWARF".

I'm not sure I understand your point.

In textual IR (lib/IR/AsmWriter.cpp & friends) we generally don't care too much about how verbose the output is, since this representation is primarily used for debugging.

From how you phrased your reply it is also possible that you were instead thinking about the in-memory representation? Currently every DIType (include/IR/DebugInfoMetadata.h) uses a unit64_t field to store the alignment.

IIUC, your goal is to avoid emitting a DW_AT_alignment attribute to types that use the default alignment. Would it make sense to store the target's default alignment in the DIType::AlignInBits field, so the DWARF backend can then decide not emit the DW_AT_alignment when it sees it?

-- adrian

Hello Adrian, sorry for the delay with the response. Somehow I missed your message…

Hello all,

I am currently implementing support for DWARFv5 DW_AT_alignment attr and I got a question about align field in debug info section of IR/Bitcode.
Thanks for looking into this!
No problem =)

Currently it is being dumped almost in any case, however according to code we use align from DI* objects only when dealing with class/structure bitfields: DwarfUnit::constructMemberDIE.

Dumping align information everywhere only for 1 case looks like overhead to me.
One your patch is finished it will be used in more cases, right? :slight_smile:
My point is that we dump align information everywhere and it is currently used only in 1 case. I am trying to make it so that it is dumped only when it is necessary.
Consider the following code:

struct S {
char c;
} s;

When compiled with debug enabled in IR we get smth like that:

!6 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: “S”, file: !5, line: 1, size: 8, align: 8, elements: !7)

Am I missing something? What is the purpose of “align: 8” here? Maybe we could include alignment information into DI* objects only when required (and thus dump it only when required): for types with bitfields and when alignas() was specified in code.
What do you mean by “include” in this context? Are you arguing for making an alignment of 8 the implicit default in textual IR? Or do you want to make DICompositeType variable length?
I mean that in this case alignment isn’t used anywhere in code further. I think we don’t need alignment information in textual DI IR in case it is not default. Dumping default alignment in every case doesn’t make sense for me: if we dump align info each time we need to mark “special” cases with additional flag in order to be able to know which case to “report” in DWARF. If we dump only “forced” alignment, we can just say: “Ok, alignment is specified, we need to put this into DWARF”.

I’m not sure I understand your point.

In textual IR (lib/IR/AsmWriter.cpp & friends) we generally don’t care too much about how verbose the output is, since this representation is primarily used for debugging.

From how you phrased your reply it is also possible that you were instead thinking about the in-memory representation? Currently every DIType (include/IR/DebugInfoMetadata.h) uses a unit64_t field to store the alignment.

IIUC, your goal is to avoid emitting a DW_AT_alignment attribute to types that use the default alignment. Would it make sense to store the target’s default alignment in the DIType::AlignInBits field, so the DWARF backend can then decide not emit the DW_AT_alignment when it sees it?

This whole thread’s got a bit long/confusing for me, but some of what’s going on might be suggestions I made earlier. My rough idea is that:

  1. We should probably only put alignment on metadata nodes that we want the DW_AT_alignment attribute to be generated for (keeps the model simple) - leave it zero otherwise.
  2. It’d be nice not to print the zero, to keep the textual representation simple/less noisy (as we do with many other metadata fields)

But it’s not a big deal if someone else feels strongly.

  • Dave

Hello Adrian, sorry for the delay with the response. Somehow I missed your message…

Hello all,

I am currently implementing support for DWARFv5 DW_AT_alignment attr and I got a question about align field in debug info section of IR/Bitcode.
Thanks for looking into this!
No problem =)

Currently it is being dumped almost in any case, however according to code we use align from DI* objects only when dealing with class/structure bitfields: DwarfUnit::constructMemberDIE.

Dumping align information everywhere only for 1 case looks like overhead to me.
One your patch is finished it will be used in more cases, right? :slight_smile:
My point is that we dump align information everywhere and it is currently used only in 1 case. I am trying to make it so that it is dumped only when it is necessary.
Consider the following code:

struct S {
char c;
} s;

When compiled with debug enabled in IR we get smth like that:

!6 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: “S”, file: !5, line: 1, size: 8, align: 8, elements: !7)

Am I missing something? What is the purpose of “align: 8” here? Maybe we could include alignment information into DI* objects only when required (and thus dump it only when required): for types with bitfields and when alignas() was specified in code.
What do you mean by “include” in this context? Are you arguing for making an alignment of 8 the implicit default in textual IR? Or do you want to make DICompositeType variable length?
I mean that in this case alignment isn’t used anywhere in code further. I think we don’t need alignment information in textual DI IR in case it is not default. Dumping default alignment in every case doesn’t make sense for me: if we dump align info each time we need to mark “special” cases with additional flag in order to be able to know which case to “report” in DWARF. If we dump only “forced” alignment, we can just say: “Ok, alignment is specified, we need to put this into DWARF”.

I’m not sure I understand your point.

In textual IR (lib/IR/AsmWriter.cpp & friends) we generally don’t care too much about how verbose the output is, since this representation is primarily used for debugging.

From how you phrased your reply it is also possible that you were instead thinking about the in-memory representation? Currently every DIType (include/IR/DebugInfoMetadata.h) uses a unit64_t field to store the alignment.

IIUC, your goal is to avoid emitting a DW_AT_alignment attribute to types that use the default alignment. Would it make sense to store the target’s default alignment in the DIType::AlignInBits field, so the DWARF backend can then decide not emit the DW_AT_alignment when it sees it?

This whole thread’s got a bit long/confusing for me, but some of what’s going on might be suggestions I made earlier. My rough idea is that:

  1. We should probably only put alignment on metadata nodes that we want the DW_AT_alignment attribute to be generated for (keeps the model simple) - leave it zero otherwise.

My thought was that we’re not saving any space by storing zero so might as well just store the actual alignment there, it might help with debugging or be otherwise useful. But — I don’t have a particular use-case in mind. If storing zero makes the model simpler, we can do that as well, either way is fine. Does COFF perhaps need the alignment?

  1. It’d be nice not to print the zero, to keep the textual representation simple/less noisy (as we do with many other metadata fields)

I’m fine with not printing zero (Victor: ideally, this be a separate commit). I’m not so comfortable with not not printing any other values (such as 8).

thanks for the explanation,
adrian

Does COFF perhaps need the alignment?

I don’t think it does, “git grep” says it has nothing to do with alignment. And according to autotests it doesn’t need it.

  1. It’d be nice not to print the zero, to keep the textual representation simple/less noisy (as we do with many other metadata fields)

I’m fine with not printing zero (Victor: ideally, this be a separate commit).

That’s already done before me, so no need for additional changes.

I’m not so comfortable with not not printing any other values (such as 8).

That’s the dilemma here: LLVM code doesn’t know whether alignment was forced by programmer or it is default one (clang does know). The first approach I tried was to print all alignment values (old behavior) and mark forced alignment with special flag (in DIFlags). After that we decided that adding new flag is overkill here and we can use alignment value instead: if it set means it was forced, otherwise it is default (current behavior). I think we should stick with the latter one as it keeps code simpler.

Does COFF perhaps need the alignment?

I don’t think it does, “git grep” says it has nothing to do with alignment. And according to autotests it doesn’t need it.

  1. It’d be nice not to print the zero, to keep the textual representation simple/less noisy (as we do with many other metadata fields)

I’m fine with not printing zero (Victor: ideally, this be a separate commit).

That’s already done before me, so no need for additional changes.

I’m not so comfortable with not not printing any other values (such as 8).

That’s the dilemma here: LLVM code doesn’t know whether alignment was forced by programmer or it is default one (clang does know). The first approach I tried was to print all alignment values (old behavior) and mark forced alignment with special flag (in DIFlags). After that we decided that adding new flag is overkill here and we can use alignment value instead: if it set means it was forced, otherwise it is default (current behavior). I think we should stick with the latter one as it keeps code simpler.

If no one else needs the alignment, I’m fine with taking this approach. Please be sure to document all this in SourceLevelDebugging.rst.

thanks!
adrian