[DWARF5]Addition of alignment field in the typedef for dwarf5

[awpandey]

Summary

This patch, adds support for DW_AT_alignment[DWARF5], to be emitted with typdef. When explicit alignment is specified.

[djtodoro]

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp

803

Please add a comment here.

llvm/test/DebugInfo/X86/debug-info-template-align.ll

27

I guess you don't want to share your local path. It is enough to write 'test.cpp' only.

40

So, we can get rid of the #N.

42

Usually, you don't need the attributes.

49

producer: "clang version 10.0.0" is enough.

50

Usually, we put an artificial name for the dir, e.g. /dir.

55

"clang version 10.0.0" is enough.

57

filename: test.c, directory: "/dir"

[djtodoro]

@awpandey Thanks for the patch!

[awpandey]

Revised and address comments of @djtodoro

[awpandey]

Thank you @djtodoro for reviewing this.

[aprantl]

clang/test/CodeGenCXX/debug-info-template-align.cpp

9

Do we need to hardcode the target here? Could we check for the specific align value?

llvm/include/llvm/IR/DIBuilder.h

243

This should be Optional<uint32_t> AlignInBits. Even better perhaps llvm::Align but perhaps that's too restrictive.

244

Do you need to update the C bindings as well?

llvm/test/DebugInfo/X86/debug-info-template-align.ll

2

RUN: llc %s -filetype=obj -o - | llvm-dwarfdump -v - | FileCheck %s

[awpandey]

@aprantl Thanks for the suggestion. Based on your suggestion I have done the following changes

1. I have added alignment value as a check in the test case
2. I have added C-Binding changes.

[awpandey]

clang/test/CodeGenCXX/debug-info-template-align.cpp

9

Sorry, I could not get your first comment

Do we need to hardcode the target here?

I have incorporated your rest of the suggestions.

llvm/include/llvm/IR/DIBuilder.h

243

Yes @aprantl this should be of Optional<unit_32t> type but changing this will require change in the DIDerivedType::get macro and this macro is used by many other functions. Please correct me if I am wrong. ??

Current functionality of the createTypeDef is like the default value of the alignInBits will be 0. Consider below comment in DIBuilder.cpp.

llvm/lib/IR/DIBuilder.cpp

309

Consider the 9th argument here

return DIDerivedType::get(VMContext, dwarf::DW_TAG_typedef, Name, File,LineNo, getNonCompileUnitScope(Context), Ty, 0, 0, --Hard coded alignment for the typedef field in existing API.
0, None, DINode::FlagZero);
}

That is why rather than changing the entire API by using Option<uint_32t> I used uint_32

[aprantl]

llvm/include/llvm/IR/DIBuilder.h

243

Updating DIDerivedType::get to use Optional would also be a good change, but for this patch I'd be happy if at least the DIBuilder interface is using optional and then treats None as 0 in the implemenation. We can optionally do another patch to change the interface of DIDerivedType::get

[awpandey]

Thanks for your suggestion @aprantl. I have revised the data type based on your suggestion. Please inform me if any other changes are required.

[aprantl]

LGTM with one change to DIBuilder inline.

llvm/include/llvm/IR/DIBuilder.h

243

This can still be Optional<unsigned> AlignInBits = {} to avoid churn.

[awpandey]

Hi @aprantl. I had made the changes and my team member will commit this.

[SouraVX]

In D70111#1748765, @awpandey wrote:

Hi @aprantl. I had made the changes and my team member will commit this.

Thanks @awpandey for working on this! as requested, I've committed / closed this.

[sammccall]

I'm reverting because this breaks ABI for llvm-c (and the go bindings).
Feel free to reapply with that change removed, though. (If this option is required for the C API, I'm not sure what the best approach is)

llvm/include/llvm-c/DebugInfo.h

878

These functions in llvm-c are ABI-stable AFAIK.
(This broke the go bindings which are in-tree, but could equally break out-of-tree bindings)

[awpandey]

Hi @aprantl. Can we drop C binding for this feature ? What will be the impact of this.
As noted by @sammccall this does not conform / break LLVM-C ABI?

[aprantl]

There are two options here:

• leave the C bindings as is (fine with me)
• add an overloaded function to the C bindings that has the extra argument (also fine with me).

[dblaikie]

I have some vague recollection that the DWARF C bindings didn't have stability guarantees? Does that sound familiar to anyone? What sort of changes have been made to them in the past?

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp

804–810

Should there be some general utility/appropriate point to add alignment to any type? (generalized over the other places we're already adding alignment on types (such as in DwarfUnit::constructTypeDIE(DIE &Buffer, const DICompositeType *CTy), maybe other places) - probably doesn't generalize over all places that add DW_AT_alignment, like global/static variables, etc, which is fine)

[aprantl]

In D70111#1752215, @dblaikie wrote:

I have some vague recollection that the DWARF C bindings didn't have stability guarantees? Does that sound familiar to anyone? What sort of changes have been made to them in the past?

That does sound familiar. I'm not sure if this was formalized anywhere or if we just all agreed on it verbally. @echristo might know?

In that case there is a 3rd option which is to

• update the C bindings and the go bindings in one commit.

[SouraVX]

Some how, this{eric response} doesn't got it's way to phabricator, anyways FYI

The C bindings, in general, don't have stability guarantees outside of a few cases.

Whitequark owns all of this now though :)

-eric

[SouraVX]

In D70111#1751981, @aprantl wrote:

There are two options here:

• leave the C bindings as is (fine with me)
• add an overloaded function to the C bindings that has the extra argument (also fine with me).

In my opinon, we should be doing both of these. Off-course Step 1 first and subsequently Step 2.
Otherwise consumers using / utilizing C bindings will again try to revert this, If we don't do Step 2.
As without an updated binding and backward compaitibilty -- this will break things for C-binding users ??

[sammccall]

My apologies if the revert was incorrect. I asked for advice after seeing breakages and heard that llvm-c should be kept ABI compatible.
But it looks like that's too strong: https://llvm.org/docs/DeveloperPolicy.html#c-api-changes says we make a best effort to maintain compatibility.

So if we don't think it's worth staying compatible here feel free to roll forward, but please do update the go bindings since they're in-tree.

[dblaikie]

Please update the go bindings and include a release note that this API is changing & why.

[awpandey]

I am considering this

• Will commit this without C bindings
• will give separate patch for C bindings and release notes (if necessary).
• will give another patch for Go bindings and release notes (if necessary).

[aprantl]

Sounds good to me!

[CodaFi]

C binding changes LGTM. Don’t worry about changing them, they are marked as experimental and subject to change at any time.

llvm/include/llvm-c/DebugInfo.h

878

The debug info bindings are not subject to the ABI stability guarantees of the rest of the C bindings to LLVM’s core.

[dblaikie]

In D70111#1754702, @awpandey wrote:

I am considering this

• Will commit this without C bindings
• will give separate patch for C bindings and release notes (if necessary).
• will give another patch for Go bindings and release notes (if necessary).

I'm not sure I follow - are you asking a question? Proposing a path forward you're planning to take (or this patch already represents)?

I'd reiterate my previous request:

Please update the go bindings and include a release note that this API is changing & why. (include that in this patch review & then it can be reviewed & all committed together to keep the whole source tree in a working state, because committing the change while either removing the C API or updating it without updating the Go bindings would break the Go bindings, which isn't good)

[awpandey]

Thanks @dblaikie. I have updated go and C bindings. I have also added the release note for why these APIs are changing.

[dblaikie]

Looks OK to me - please recommit when you're ready.

[SouraVX]

In D70111#1765223, @dblaikie wrote:

Looks OK to me - please recommit when you're ready.

Thank you @awpandey . As requested, I've recommited this.

[djtodoro]

llvm/docs/ReleaseNotes.rst

169 ↗

(On Diff #231004)

It looks like this breaks the llvm-sphinx-docs build.

[djtodoro]

llvm/docs/ReleaseNotes.rst

169 ↗

(On Diff #231004)

Fixed with the 5762648.

Please take a look into the http://lab.llvm.org:8011/console when committing changes. There you can find info about build bots failures, if any.

[SouraVX]

llvm/docs/ReleaseNotes.rst

169 ↗

(On Diff #231004)

Thank You @djtodoro , had a brief look at 5762648. Seems good.
BTW are sphinx bots still failing due to this ?

[djtodoro]

llvm/docs/ReleaseNotes.rst

169 ↗

(On Diff #231004)

The builder is back to passing again.

clang/lib/CodeGen/CGDebugInfo.cpp

   SourceLocation Loc = AliasDecl->getLocation();
   return DBuilder.createTypedef(Src, OS.str(), getOrCreateFile(Loc),
                                 getLineNumber(Loc),
-                                getDeclContextDescriptor(AliasDecl));
+                                getDeclContextDescriptor(AliasDecl),
+                                /* Alignment */ None);
 }
 
 llvm::DIType *CGDebugInfo::CreateType(const TypedefType *Ty,

   SourceLocation Loc = Ty->getDecl()->getLocation();
 
   // Typedefs are derived from some other type.
-  return DBuilder.createTypedef(Underlying, Ty->getDecl()->getName(),
-                                getOrCreateFile(Loc), getLineNumber(Loc),
-                                getDeclContextDescriptor(Ty->getDecl()));
+  return DBuilder.createTypedef(
+      Underlying, Ty->getDecl()->getName(), getOrCreateFile(Loc),
+      getLineNumber(Loc), getDeclContextDescriptor(Ty->getDecl()),
+      getDeclAlignIfRequired(Ty->getDecl(), CGM.getContext()));
 }
 
 static unsigned getDwarfCC(CallingConv CC) {

   return DBuilder.createTypedef(
       getOrCreateType(Ty->getDecl()->getUnderlyingType(), Unit),
       Ty->getDecl()->getName(), getOrCreateFile(Loc), getLineNumber(Loc),
-      getDeclContextDescriptor(Ty->getDecl()));
+      getDeclContextDescriptor(Ty->getDecl()),
+      /* Alignment */ None);
 }
 
 /// \return true if Getter has the default name for the property PD.

clang/test/CodeGenCXX/debug-info-template-align.cpp

+//  Test for debug info related to DW_AT_alignment attribute in the typedef operator
+// Supported: -O0, standalone DI
+// RUN: %clang_cc1 -dwarf-version=5  -emit-llvm -triple x86_64-linux-gnu %s -o - \
+// RUN:   -O0 -disable-llvm-passes \
+// RUN:   -debug-info-kind=standalone \
+// RUN: | FileCheck %s
+
+// CHECK: DIDerivedType(tag: DW_TAG_typedef, {{.*}}, align: 512
+
+typedef char __attribute__((__aligned__(64))) alchar;
+
+int main() {
+  alchar newChar;
+}

llvm/include/llvm-c/DebugInfo.h

 LLVMDIBuilderCreateTypedef(LLVMDIBuilderRef Builder, LLVMMetadataRef Type,
                            const char *Name, size_t NameLen,
                            LLVMMetadataRef File, unsigned LineNo,
-                           LLVMMetadataRef Scope);
+                           LLVMMetadataRef Scope, uint32_t AlignInBits);
 
 /**
  * Create debugging information entry to establish inheritance relationship

llvm/include/llvm/IR/DIBuilder.h

     /// \param File        File where this type is defined.
     /// \param LineNo      Line number.
     /// \param Context     The surrounding context for the typedef.
+    /// \param AlignInBits Alignment. (optional)
     DIDerivedType *createTypedef(DIType *Ty, StringRef Name, DIFile *File,
-                                 unsigned LineNo, DIScope *Context);
+                                 unsigned LineNo, DIScope *Context,
+                                 Optional<unsigned> AlignInBits);
 
     /// Create debugging information entry for a 'friend'.
     DIDerivedType *createFriend(DIType *Ty, DIType *FriendTy);

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp

   if (!Name.empty())
     addString(Buffer, dwarf::DW_AT_name, Name);
 
+  // If alignment is specified for a typedef , create and insert DW_AT_alignment
+  // attribute in DW_TAG_typedef DIE.
+  if (Tag == dwarf::DW_TAG_typedef && DD->getDwarfVersion() >= 5) {
+    uint32_t AlignInBytes = DTy->getAlignInBytes();
+    if (AlignInBytes > 0)
+      addUInt(Buffer, dwarf::DW_AT_alignment, dwarf::DW_FORM_udata,
+              AlignInBytes);
+  }
+
   // Add size if non-zero (derived types might be zero-sized.)
   if (Size && Tag != dwarf::DW_TAG_pointer_type
            && Tag != dwarf::DW_TAG_ptr_to_member_type

llvm/lib/IR/DIBuilder.cpp

 
 DIDerivedType *DIBuilder::createTypedef(DIType *Ty, StringRef Name,
                                         DIFile *File, unsigned LineNo,
-                                        DIScope *Context) {
+                                        DIScope *Context,
+                                        Optional<unsigned> AlignInBits) {
   return DIDerivedType::get(VMContext, dwarf::DW_TAG_typedef, Name, File,
-                            LineNo, getNonCompileUnitScope(Context), Ty, 0, 0,
+                            LineNo, getNonCompileUnitScope(Context), Ty, 0,
+                            AlignInBits.hasValue() ? AlignInBits.getValue() : 0,
                             0, None, DINode::FlagZero);
 }
 

llvm/lib/IR/DebugInfo.cpp

 LLVMDIBuilderCreateTypedef(LLVMDIBuilderRef Builder, LLVMMetadataRef Type,
                            const char *Name, size_t NameLen,
                            LLVMMetadataRef File, unsigned LineNo,
-                           LLVMMetadataRef Scope) {
+                           LLVMMetadataRef Scope, uint32_t AlignInBits) {
   return wrap(unwrap(Builder)->createTypedef(
-                  unwrapDI<DIType>(Type), {Name, NameLen},
-                  unwrapDI<DIFile>(File), LineNo,
-                  unwrapDI<DIScope>(Scope)));
+      unwrapDI<DIType>(Type), {Name, NameLen}, unwrapDI<DIFile>(File), LineNo,
+      unwrapDI<DIScope>(Scope), AlignInBits));
 }
 
 LLVMMetadataRef

llvm/test/DebugInfo/X86/debug-info-template-align.ll

+; RUN: llc %s -filetype=obj -o - | llvm-dwarfdump -v - | FileCheck %s
+
+; C++ source to regenerate:
+
+;typedef char  __attribute__((__aligned__(64))) alchar;
+
+;int main(){
+;    alchar newChar;
+;}
+; $ clang++ -O0 -g -gdwarf-5 debug-info-template-align.cpp -c
+
+; CHECK: .debug_abbrev contents:
+
+; CHECK: [5] DW_TAG_typedef  DW_CHILDREN_no
+; CHECK:  DW_AT_alignment DW_FORM_udata
+
+; CHECK: .debug_info contents:
+
+;CHECK: DW_TAG_typedef [5]
+;CHECK: DW_AT_name {{.*}} "alchar"
+;CHECK-NEXT: DW_AT_alignment [DW_FORM_udata] (64)
+
+
+; ModuleID = '/dir/test.cpp'
+source_filename = "/dir/test.cpp"
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: noinline norecurse nounwind optnone uwtable
+define dso_local i32 @main() #0 !dbg !7 {
+entry:
+  %newChar = alloca i8, align 64
+  call void @llvm.dbg.declare(metadata i8* %newChar, metadata !12, metadata !DIExpression()), !dbg !15
+  ret i32 0, !dbg !16
+}
+
+; Function Attrs: nounwind readnone speculatable willreturn
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+attributes #0 = { noinline norecurse nounwind optnone uwtable }
+attributes #1 = { nounwind readnone speculatable willreturn }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang version 10.0.0 ", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, nameTableKind: None)
+!1 = !DIFile(filename: "/dir/test.cpp", directory: "/home/awpandey/tools/llvm/test/DebugInfo", checksumkind: CSK_MD5, checksum: "872e252efdfcb9480b4bfaf8437f58ab")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 5}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{!"clang version 10.0.0 "}
+!7 = distinct !DISubprogram(name: "main", scope: !8, file: !8, line: 12, type: !9, scopeLine: 12, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
+!8 = !DIFile(filename: "test.cpp", directory: "/dir", checksumkind: CSK_MD5, checksum: "872e252efdfcb9480b4bfaf8437f58ab")
+!9 = !DISubroutineType(types: !10)
+!10 = !{!11}
+!11 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!12 = !DILocalVariable(name: "newChar", scope: !7, file: !8, line: 13, type: !13)
+!13 = !DIDerivedType(tag: DW_TAG_typedef, name: "alchar", file: !8, line: 10, baseType: !14, align: 512)
+!14 = !DIBasicType(name: "char", size: 8, encoding: DW_ATE_signed_char)
+!15 = !DILocation(line: 13, column: 10, scope: !7)
+!16 = !DILocation(line: 14, column: 1, scope: !7)

llvm/tools/llvm-c-test/debuginfo.c

   LLVMMetadataRef Int64Ty =
       LLVMDIBuilderCreateBasicType(DIB, "Int64", 5, 64, 0, LLVMDIFlagZero);
   LLVMMetadataRef Int64TypeDef =
-    LLVMDIBuilderCreateTypedef(DIB, Int64Ty, "int64_t", 7, File, 42, File);
+      LLVMDIBuilderCreateTypedef(DIB, Int64Ty, "int64_t", 7, File, 42, File, 0);
 
   LLVMMetadataRef GlobalVarValueExpr =
       LLVMDIBuilderCreateConstantValueExpression(DIB, 0);