Page MenuHomePhabricator

DebugInfo: support for DWARFv5 DW_AT_alignment attribute
ClosedPublic

Authored by vleschuk on Sep 9 2016, 1:53 PM.

Details

Summary
  • Assume that clang passes non-zero alignment value to DIBuilder only in case when it was forced by C++11 'alignas', C11 '_Alignas' or compiler attribute '__attribute((aligned (N)))'.
  • Emit DW_AT_alignment if alignment is specified for type/object. See: http://www.dwarfstd.org/ShowIssue.php?issue=140528.1 for more details.

Diff Detail

Repository
rL LLVM

Event Timeline

vleschuk updated this revision to Diff 70896.Sep 9 2016, 1:53 PM
vleschuk retitled this revision from to DebugInfo: support for DWARFv5 DW_AT_alignment attribute.
vleschuk updated this object.
vleschuk added a subscriber: llvm-commits.
dblaikie edited edge metadata.Sep 9 2016, 2:56 PM

What's the purpose of the flag? Could we just use the non-zero value of the alignment attribute as the signal of whether to emit the DW_AT_alignment attribute? (eg: if the alignment is specified, emit it - simple?)

DW_FORM_data8 seems wasteful. I'd rather see DW_FORM_udata.

What's the purpose of the flag? Could we just use the non-zero value of the alignment attribute as the signal of whether to emit the DW_AT_alignment attribute? (eg: if the alignment is specified, emit it - simple?)

The alignment could be not specified and default alignment is used. Consider the following example:

struct S {
} s;

This will result in the following IR code:

!5 = !DICompositeType(tag: DW_TAG_structure_type, name: "S", file: !1, line: 1, align: 8, elements: !2)

And getAlignInBits() will return 8. Thus we need to mark entities with non-default alignment.

DW_FORM_data8 seems wasteful. I'd rather see DW_FORM_udata.

Agreed. I also had a thought that data8 is too large for that. I just (for some reason) was sure that DWARF document explicitly stated that value should be 64 bit. Rechecked now: there is no such info in that doc, I was mistaken. Will fix.

vleschuk updated this revision to Diff 70950.Sep 9 2016, 9:10 PM
vleschuk edited edge metadata.

Switched from DW_FORM_data8 to DW_FORM_udata for DW_AT_alignment.

What's the purpose of the flag? Could we just use the non-zero value of the alignment attribute as the signal of whether to emit the DW_AT_alignment attribute? (eg: if the alignment is specified, emit it - simple?)

The alignment could be not specified and default alignment is used. Consider the following example:

struct S {
} s;

This will result in the following IR code:

!5 = !DICompositeType(tag: DW_TAG_structure_type, name: "S", file: !1, line: 1, align: 8, elements: !2)

And getAlignInBits() will return 8. Thus we need to mark entities with non-default alignment.

Is the alignment used for anything in this case? Could we just remove it & make it so that whenever it's present we put the DW_AT_alignment attribute on the entity?

What's the purpose of the flag? Could we just use the non-zero value of the alignment attribute as the signal of whether to emit the DW_AT_alignment attribute? (eg: if the alignment is specified, emit it - simple?)

The alignment could be not specified and default alignment is used. Consider the following example:

struct S {
} s;

This will result in the following IR code:

!5 = !DICompositeType(tag: DW_TAG_structure_type, name: "S", file: !1, line: 1, align: 8, elements: !2)

And getAlignInBits() will return 8. Thus we need to mark entities with non-default alignment.

Is the alignment used for anything in this case? Could we just remove it & make it so that whenever it's present we put the DW_AT_alignment attribute on the entity?

The DW_AT_alignment is intended to be emitted only if object's alignment is not default. In case above the default alignment for struct S is 1 byte but C++11 and C11 (or compiler-specific directives) allows user to specify different alignment value.

Alignment value (whether it is default or user-defined) must be present in IR in order to be able to generate correct code, as for new DWARF attribute: we just give debuggers information that user has explicitly set the alignment value. This knowledge can be useful in case a debugger needs to create an object for use with some expressions to be evaluated.

In D24425#539028, @dblaikie wrote:

Alignment value (whether it is default or user-defined) must be present in IR in order to be able to generate correct code,

I'm not sure I follow this ^ - the metadata here is only for debug info. It's not used for correctness of the resulting program. (the alignment of pointers in the IR for actual code generation is stored separately).

Correct. Debug Info dump doesn't influence code generation. Bad example from my side.

I'll try to rephrase:

Is the alignment currently emitted in the debug info IR metadata used for anything? Seems it's used for bitfield related things, but perhaps that's not the right solution/we could do that another way? So that the alignment just corresponds directly to the DW_AT_alignment (eg: when present, emit an alignment attribute, otherwise don't) to keep the metadata simple.

Ah, I got it now. You suggest the following way:

  • Do not add DINode::FlagAlignment
  • Do not add alignment to DI* objects when creating them unless alignment was forced by user
  • When generating DWARF output emit DW_AT_alignment if DI* object has non-zero alignment field

I have looked through the code and you seem to be correct, actually the align value from DI* objects is used only when working with bitfields. I think I should consult with llvm-dev@ before breaking anything.

vleschuk updated this revision to Diff 70964.Sep 11 2016, 8:59 AM
vleschuk updated this object.

Got rid of DINode::FlagAlignment: now we assume that clang passes non-zero align value to DIBuilder only in case if alignment was forced. Thus we just check for non-zero align value and if true, emit DW_AT_alignment. Also updated related tests according to new behavior.

This might be a bit easier to review if it were broken into separate patches - it looks like there's orthogonal functionality in here (ie: some code to add the alignment attribute to global variables, some to add it to types, etc) - is that the case? Do those changes have some pieces in common that needs to be staged together in some way?

This might be a bit easier to review if it were broken into separate patches - it looks like there's orthogonal functionality in here (ie: some code to add the alignment attribute to global variables, some to add it to types, etc) - is that the case? Do those changes have some pieces in common that needs to be staged together in some way?

Yep, some code can be moved to separate patch. Will create a new review and put it into relationship with this one. Thanks for advice.

vleschuk updated this revision to Diff 73226.Oct 2 2016, 6:25 PM
aprantl added inline comments.Oct 4 2016, 8:54 AM
include/llvm/IR/DIBuilder.h
139 ↗(On Diff #73226)

What is the motivation to change this to bytes? This kind of API change looks dangerous to me because frontends won't even get a type error to notify them that the API has changed.

vleschuk added inline comments.Oct 4 2016, 9:47 AM
include/llvm/IR/DIBuilder.h
139 ↗(On Diff #73226)

Hmm, yep, I think you are right. I thought it would be better to pass bytes from frontend to backend as frontend has special CharUnits class for this purpose, however such changes in API can affect another frontends. Will change it back to bits and convert in backend.

vleschuk updated this revision to Diff 73536.Oct 4 2016, 12:48 PM

Revert API changes: DIBuilder create* methods now take alignment in bits instead of bytes, like before (conversion is done when emitting DW_AT_alignment).

vleschuk marked 2 inline comments as done.Oct 4 2016, 12:50 PM
aprantl added inline comments.Oct 4 2016, 1:10 PM
include/llvm/IR/DIBuilder.h
212 ↗(On Diff #73536)

This should be a separate NFC commit.

lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
779 ↗(On Diff #73536)

Can this ever be 0?

lib/Support/Dwarf.cpp
152 ↗(On Diff #73536)

For consistency with the rest of the file, I would put this on the same line.

test/DebugInfo/X86/align_c11.ll
52 ↗(On Diff #73536)

We typically get rid of all unnecessary attributes (= everything in quotes).

test/DebugInfo/X86/align_cpp11.ll
114 ↗(On Diff #73536)

same here

unittests/Transforms/Utils/Cloning.cpp
256 ↗(On Diff #73536)

This should be a separate NFC commit.

vleschuk added inline comments.Oct 6 2016, 1:37 AM
lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
779 ↗(On Diff #73536)

I think it can be NULL: class DbgVariable doesn't check its DILocalVariable * member for being NULL when being constructed or accessed.

lib/Support/Dwarf.cpp
152 ↗(On Diff #73536)

See DW_AT_MIPS_software_pipeline_depth it has same formatting (as requires coding standard and clang-format), so we already have this file formatting inconsistent. I'd prefer keeping clang-format silent on changes rather them keeping current formatting intact manually.

vleschuk marked 8 inline comments as done.Oct 6 2016, 5:08 AM
vleschuk updated this revision to Diff 73775.Oct 6 2016, 5:41 AM
  • Reverted unnecessary format changes
  • Removed unnecessary context from tests
  • Updated documentation for SourceLevelDebugging.rst
aprantl added inline comments.Oct 6 2016, 9:38 AM
docs/SourceLevelDebugging.rst
450 ↗(On Diff #73775)

Thanks!!

lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
779 ↗(On Diff #73536)

Hmm. I don't think it makes sense to ever construct a DbgVariable without a variable. We should probably change it to a reference at some point. Thanks for checking.

lib/CodeGen/AsmPrinter/DwarfUnit.cpp
1415 ↗(On Diff #73775)

What about packed structs?

Sorry, I'm just being very cautious about any changes here, because every single time I touched this code in that past I broke the either LLDB or GDB ;-)

A couple of examples from the testsuite you can run through clang with both -gdwarf-2 and -gdwarf-4 and verify that the DWARF output didn't change:

#pragma pack(1)
    struct PackedBits
     {
        char a;
        uint32_t b : 5,
        c : 27
      } s;
   #pragma pack()

 struct bitfield {
    int a : 2;
    int b : 32;
    int c : 1;
    int d : 28;
}

struct size8 {
  int i : 4;
  long long l : 60;
};


struct layout0 {
  char l0_ofs0;
  struct size8 l0_ofs8;
  int l0_ofs16 : 1;
};

struct size8_anon {
  int : 4;
  long long : 60;
};
struct layout1 {
  char l1_ofs0;
  struct size8_anon l1_ofs1;
  int l1_ofs9 : 1;
};

#pragma pack(1)
struct size8_pack1 {
  int i : 4;
  long long l : 60;
};
struct layout2 {
  char l2_ofs0;
  struct size8_pack1 l2_ofs1;
  int l2_ofs9 : 1;
};
#pragma pack()

#pragma pack(4)
struct size8_pack4 {
  int i : 4;
  long long l : 60;
};
struct layout3 {
  char l3_ofs0;
  struct size8_pack4 l3_ofs4;
  int l3_ofs12 : 1;
};
#pragma pack()
vleschuk marked 3 inline comments as done.Oct 10 2016, 2:54 AM
vleschuk added inline comments.
lib/CodeGen/AsmPrinter/DwarfUnit.cpp
1415 ↗(On Diff #73775)

We already have test for that: DebugInfo/X86/debug-info-packed-struct.ll - its based on debug-info-packed-struct.c from clang (the one you quoted). I also manually checked this both with dwarf-2 and dwarf-4, dwarf output is the same for clang 3.9 release and current build with my changes.

vleschuk updated this revision to Diff 74114.Oct 10 2016, 2:55 AM
vleschuk marked an inline comment as done.
  • Fix mistype in SourceLevelDebugging.rst (attribute() -> attribute__()).
aprantl accepted this revision.Oct 10 2016, 12:58 PM
aprantl edited edge metadata.

Thanks, this looks good to me!

This revision is now accepted and ready to land.Oct 10 2016, 12:58 PM
vleschuk updated this revision to Diff 74689.Oct 14 2016, 7:43 AM
vleschuk edited edge metadata.
  • Use DIAlignment type instead of uint64_t for alignment in DebugInfo
This revision was automatically updated to reflect the committed changes.
  • Use DIAlignment type instead of uint64_t for alignment in DebugInfo

DIAlignment was never typedef'd so causes compile failures ;-(