[PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced

2016-10-19 Thread Victor Leschuk via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rL284679: DebugInfo: pass alignment value only if it was 
forced (authored by vleschuk).

Changed prior to commit:
  https://reviews.llvm.org/D24426?vs=74686&id=75261#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D24426

Files:
  cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
  cfe/trunk/lib/CodeGen/CGDebugInfo.h
  cfe/trunk/test/CodeGen/debug-info-packed-struct.c
  cfe/trunk/test/CodeGen/debug-info-vector.c
  cfe/trunk/test/CodeGenCXX/debug-info-calling-conventions.cpp
  cfe/trunk/test/CodeGenCXX/debug-info-enum-class.cpp
  cfe/trunk/test/CodeGenCXX/debug-info-indirect-field-decl.cpp
  cfe/trunk/test/CodeGenCXX/debug-info-ms-bitfields.cpp
  cfe/trunk/test/CodeGenCXX/debug-info-ms-ptr-to-member.cpp
  cfe/trunk/test/CodeGenCXX/debug-info-rvalue-ref.cpp
  cfe/trunk/test/CodeGenCXX/debug-info-template-quals.cpp
  cfe/trunk/test/CodeGenCXX/debug-info-template.cpp
  cfe/trunk/test/CodeGenCXX/debug-info-union.cpp
  cfe/trunk/test/CodeGenCXX/debug-info-uuid.cpp
  cfe/trunk/test/CodeGenCXX/debug-info-vla.cpp
  cfe/trunk/test/CodeGenCXX/debug-info-zero-length-arrays.cpp
  cfe/trunk/test/CodeGenCXX/debug-info.cpp
  cfe/trunk/test/CodeGenCXX/debug-lambda-this.cpp
  cfe/trunk/test/CodeGenObjC/block-byref-debuginfo.m
  cfe/trunk/test/CodeGenObjC/debug-info-block-type.m
  cfe/trunk/test/CodeGenObjC/debug-info-ivars-extension.m
  cfe/trunk/test/CodeGenObjC/debug-info-ivars-private.m
  cfe/trunk/test/CodeGenObjC/debug-info-ivars.m
  cfe/trunk/test/CodeGenObjCXX/debug-info-cyclic.mm

Index: cfe/trunk/test/CodeGen/debug-info-packed-struct.c
===
--- cfe/trunk/test/CodeGen/debug-info-packed-struct.c
+++ cfe/trunk/test/CodeGen/debug-info-packed-struct.c
@@ -19,9 +19,9 @@
 };
 // CHECK: l0_ofs0
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "l0_ofs8",
-// CHECK-SAME: {{.*}}size: 64, align: 64, offset: 64)
+// CHECK-SAME: {{.*}}size: 64, offset: 64)
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "l0_ofs16",
-// CHECK-SAME: {{.*}}size: 1, align: 32, offset: 128, flags: DIFlagBitField, extraData: i64 128)
+// CHECK-SAME: {{.*}}size: 1, offset: 128, flags: DIFlagBitField, extraData: i64 128)
 
 
 // -
@@ -38,9 +38,9 @@
 };
 // CHECK: l1_ofs0
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "l1_ofs1",
-// CHECK-SAME: {{.*}}size: 64, align: 8, offset: 8)
+// CHECK-SAME: {{.*}}size: 64, offset: 8)
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "l1_ofs9",
-// CHECK-SAME: {{.*}}size: 1, align: 32, offset: 72, flags: DIFlagBitField, extraData: i64 72)
+// CHECK-SAME: {{.*}}size: 1, offset: 72, flags: DIFlagBitField, extraData: i64 72)
 
 
 // -
@@ -59,9 +59,9 @@
 #pragma pack()
 // CHECK: l2_ofs0
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "l2_ofs1",
-// CHECK-SAME: {{.*}}size: 64, align: 8, offset: 8)
+// CHECK-SAME: {{.*}}size: 64, offset: 8)
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "l2_ofs9",
-// CHECK-SAME: {{.*}}size: 1, align: 32, offset: 72, flags: DIFlagBitField, extraData: i64 72)
+// CHECK-SAME: {{.*}}size: 1, offset: 72, flags: DIFlagBitField, extraData: i64 72)
 
 
 
@@ -81,9 +81,9 @@
 #pragma pack()
 // CHECK: l3_ofs0
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "l3_ofs4",
-// CHECK-SAME: {{.*}}size: 64, align: 32, offset: 32)
+// CHECK-SAME: {{.*}}size: 64, offset: 32)
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "l3_ofs12",
-// CHECK-SAME: {{.*}}size: 1, align: 32, offset: 96, flags: DIFlagBitField, extraData: i64 96)
+// CHECK-SAME: {{.*}}size: 1, offset: 96, flags: DIFlagBitField, extraData: i64 96)
 
 struct layout3 l3;
 struct layout0 l0;
Index: cfe/trunk/test/CodeGen/debug-info-vector.c
===
--- cfe/trunk/test/CodeGen/debug-info-vector.c
+++ cfe/trunk/test/CodeGen/debug-info-vector.c
@@ -6,6 +6,6 @@
 // Test that we get an array type that's also a vector out of debug.
 // CHECK: !DICompositeType(tag: DW_TAG_array_type,
 // CHECK-SAME: baseType: ![[INT:[0-9]+]]
-// CHECK-SAME: size: 128, align: 128
+// CHECK-SAME: size: 128
 // CHECK-SAME: DIFlagVector
 // CHECK: ![[INT]] = !DIBasicType(name: "int"
Index: cfe/trunk/test/CodeGenObjC/block-byref-debuginfo.m
===
--- cfe/trunk/test/CodeGenObjC/block-byref-debuginfo.m
+++ cfe/trunk/test/CodeGenObjC/block-byref-debuginfo.m
@@ -5,7 +5,6 @@
 // expression (256) that locates it inside of the byref descriptor:
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "foo",
 // CHECK-NOT:line:
-// CHECK-SAME:   align: 64
 // CHECK-SAME:   offset: 256
 
 struct Foo {
Index: cfe/trunk/test/C

[PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced

2016-10-02 Thread Victor Leschuk via cfe-commits 
vleschuk updated this revision to Diff 73228.
vleschuk added a comment.

- Move alignment detection into helper functions in anon namespace
- Use updated LLVM DIBuilder API (from https://reviews.llvm.org/D24425)


https://reviews.llvm.org/D24426

Files:
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/CGDebugInfo.h
  test/CodeGen/debug-info-packed-struct.c
  test/CodeGen/debug-info-vector.c
  test/CodeGenCXX/debug-info-calling-conventions.cpp
  test/CodeGenCXX/debug-info-enum-class.cpp
  test/CodeGenCXX/debug-info-indirect-field-decl.cpp
  test/CodeGenCXX/debug-info-ms-bitfields.cpp
  test/CodeGenCXX/debug-info-ms-ptr-to-member.cpp
  test/CodeGenCXX/debug-info-rvalue-ref.cpp
  test/CodeGenCXX/debug-info-template-quals.cpp
  test/CodeGenCXX/debug-info-template.cpp
  test/CodeGenCXX/debug-info-union.cpp
  test/CodeGenCXX/debug-info-uuid.cpp
  test/CodeGenCXX/debug-info-vla.cpp
  test/CodeGenCXX/debug-info-zero-length-arrays.cpp
  test/CodeGenCXX/debug-info.cpp
  test/CodeGenCXX/debug-lambda-this.cpp
  test/CodeGenObjC/block-byref-debuginfo.m
  test/CodeGenObjC/debug-info-block-type.m
  test/CodeGenObjC/debug-info-ivars-extension.m
  test/CodeGenObjC/debug-info-ivars-private.m
  test/CodeGenObjC/debug-info-ivars.m
  test/CodeGenObjCXX/debug-info-cyclic.mm

Index: test/CodeGenObjCXX/debug-info-cyclic.mm
===
--- test/CodeGenObjCXX/debug-info-cyclic.mm
+++ test/CodeGenObjCXX/debug-info-cyclic.mm
@@ -3,7 +3,7 @@
 struct B {
 // CHECK: ![[B:[0-9]+]] ={{.*}}!DICompositeType(tag: DW_TAG_structure_type, name: "B"
 // CHECK-SAME: line: [[@LINE-2]],
-// CHECK-SAME: size: 8, align: 8,
+// CHECK-SAME: size: 8,
 // CHECK-NOT:  offset:
 // CHECK-NOT:  DIFlagFwdDecl
 // CHECK-SAME: elements: ![[BMEMBERS:[0-9]+]]
Index: test/CodeGenObjC/debug-info-ivars.m
===
--- test/CodeGenObjC/debug-info-ivars.m
+++ test/CodeGenObjC/debug-info-ivars.m
@@ -21,24 +21,24 @@
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "i"
 // CHECK-SAME:   line: 10
 // CHECK-SAME:   baseType: ![[INT:[0-9]+]]
-// CHECK-SAME:   size: 32, align: 32,
+// CHECK-SAME:   size: 32,
 // CHECK-NOT:offset:
 // CHECK-SAME:   flags: DIFlagProtected
 // CHECK: ![[INT]] = !DIBasicType(name: "int"
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_1"
 // CHECK-SAME:   line: 11
 // CHECK-SAME:   baseType: ![[UNSIGNED:[0-9]+]]
-// CHECK-SAME:   size: 9, align: 32,
+// CHECK-SAME:   size: 9,
 // CHECK-NOT:offset:
 // CHECK-SAME:   flags: DIFlagProtected
 // CHECK: ![[UNSIGNED]] = !DIBasicType(name: "unsigned int"
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_2"
 // CHECK-SAME:   line: 12
 // CHECK-SAME:   baseType: ![[UNSIGNED]]
-// CHECK-SAME:   size: 9, align: 32, offset: 1,
+// CHECK-SAME:   size: 9, offset: 1,
 // CHECK-SAME:   flags: DIFlagProtected
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_3"
 // CHECK-SAME:   line: 14
 // CHECK-SAME:   baseType: ![[UNSIGNED]]
-// CHECK-SAME:   size: 9, align: 32, offset: 3,
+// CHECK-SAME:   size: 9, offset: 3,
 // CHECK-SAME:   flags: DIFlagProtected
Index: test/CodeGenObjC/debug-info-ivars-private.m
===
--- test/CodeGenObjC/debug-info-ivars-private.m
+++ test/CodeGenObjC/debug-info-ivars-private.m
@@ -35,13 +35,13 @@
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "foo"
 // CHECK-SAME:   line: 14
 // CHECK-SAME:   baseType: ![[INT:[0-9]+]]
-// CHECK-SAME:   size: 32, align: 32,
+// CHECK-SAME:   size: 32,
 // CHECK-NOT:offset:
 // CHECK-SAME:   flags: DIFlagProtected
 // CHECK: ![[INT]] = !DIBasicType(name: "int"
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "bar"
 // CHECK-SAME:   line: 27
 // CHECK-SAME:   baseType: ![[INT:[0-9]+]]
-// CHECK-SAME:   size: 32, align: 32,
+// CHECK-SAME:   size: 32,
 // CHECK-NOT:offset:
 // CHECK-SAME:   flags: DIFlagPrivate
Index: test/CodeGenObjC/debug-info-ivars-extension.m
===
--- test/CodeGenObjC/debug-info-ivars-extension.m
+++ test/CodeGenObjC/debug-info-ivars-extension.m
@@ -30,7 +30,7 @@
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "a"
 // CHECK-SAME:   line: 7
 // CHECK-SAME:   baseType: ![[INT:[0-9]+]]
-// CHECK-SAME:   size: 32, align: 32
+// CHECK-SAME:   size: 32
 // CHECK-NOT:offset:
 // CHECK-SAME:   flags: DIFlagPublic
 // CHECK: ![[INT]] = !DIBasicType(name: "int"
@@ -42,6 +42,6 @

[PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced

2016-10-02 Thread Victor Leschuk via cfe-commits 
vleschuk marked 3 inline comments as done.
vleschuk added inline comments.


> vleschuk wrote in CGDebugInfo.cpp:608
> Will check if this works in all cases. I think it's worth putting this 
> snippet into helper function within anon namespace.

That is correct for types, for particular decls we still need to use hasAttr<>. 
Moved both cases into separate functions.

> dblaikie wrote in CGDebugInfo.cpp:609
> Maybe add a comment here about why we're specifying zero alignment (or 
> possibly change the API to not take an alignment at all so we don't have to 
> explain it?)
> 
> (similarly for other cases where the alignment is just hardcoded to zero)

Default arguments now are zeros.

https://reviews.llvm.org/D24426



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced

2016-10-03 Thread Reid Kleckner via cfe-commits 
rnk added inline comments.


> CGDebugInfo.cpp:47
>  
> +namespace {
> +template 

LLVM prefers `static` to anonymous namespaces 
http://llvm.org/docs/CodingStandards.html#anonymous-namespaces

> CGDebugInfo.cpp:48
> +namespace {
> +template 
> +uint64_t GetTypeAlignIfRequired(Type Ty, const ASTContext &Ctx) {

Instead of templating, have a `const Type*` overload and a QualType overload 
like getTypeInfo does:

  /// \brief Get the size and alignment of the specified complete type in bits.
  TypeInfo getTypeInfo(const Type *T) const;
  TypeInfo getTypeInfo(QualType T) const { return getTypeInfo(T.getTypePtr()); }

> CGDebugInfo.cpp:49
> +template 
> +uint64_t GetTypeAlignIfRequired(Type Ty, const ASTContext &Ctx) {
> +  auto TI = Ctx.getTypeInfo(Ty);

LLVM style uses a leading lower case for method names: 
http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

> CGDebugInfo.cpp:53
> +: 0;
> +}
> +uint64_t GetDeclAlignIfRequired(const Decl *D, const ASTContext &Ctx) {

Please add a blank line between top level decls

https://reviews.llvm.org/D24426



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced

2016-10-03 Thread Victor Leschuk via cfe-commits 
vleschuk updated this revision to Diff 73318.
vleschuk added a comment.

Fix code style:

- Two overloaded methods instead of 1 template
- lower-case-headed method names
- static methods instead of anon namespace
- extra new line between methods


https://reviews.llvm.org/D24426

Files:
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/CGDebugInfo.h
  test/CodeGen/debug-info-packed-struct.c
  test/CodeGen/debug-info-vector.c
  test/CodeGenCXX/debug-info-calling-conventions.cpp
  test/CodeGenCXX/debug-info-enum-class.cpp
  test/CodeGenCXX/debug-info-indirect-field-decl.cpp
  test/CodeGenCXX/debug-info-ms-bitfields.cpp
  test/CodeGenCXX/debug-info-ms-ptr-to-member.cpp
  test/CodeGenCXX/debug-info-rvalue-ref.cpp
  test/CodeGenCXX/debug-info-template-quals.cpp
  test/CodeGenCXX/debug-info-template.cpp
  test/CodeGenCXX/debug-info-union.cpp
  test/CodeGenCXX/debug-info-uuid.cpp
  test/CodeGenCXX/debug-info-vla.cpp
  test/CodeGenCXX/debug-info-zero-length-arrays.cpp
  test/CodeGenCXX/debug-info.cpp
  test/CodeGenCXX/debug-lambda-this.cpp
  test/CodeGenObjC/block-byref-debuginfo.m
  test/CodeGenObjC/debug-info-block-type.m
  test/CodeGenObjC/debug-info-ivars-extension.m
  test/CodeGenObjC/debug-info-ivars-private.m
  test/CodeGenObjC/debug-info-ivars.m
  test/CodeGenObjCXX/debug-info-cyclic.mm

Index: test/CodeGenObjCXX/debug-info-cyclic.mm
===
--- test/CodeGenObjCXX/debug-info-cyclic.mm
+++ test/CodeGenObjCXX/debug-info-cyclic.mm
@@ -3,7 +3,7 @@
 struct B {
 // CHECK: ![[B:[0-9]+]] ={{.*}}!DICompositeType(tag: DW_TAG_structure_type, name: "B"
 // CHECK-SAME: line: [[@LINE-2]],
-// CHECK-SAME: size: 8, align: 8,
+// CHECK-SAME: size: 8,
 // CHECK-NOT:  offset:
 // CHECK-NOT:  DIFlagFwdDecl
 // CHECK-SAME: elements: ![[BMEMBERS:[0-9]+]]
Index: test/CodeGenObjC/debug-info-ivars.m
===
--- test/CodeGenObjC/debug-info-ivars.m
+++ test/CodeGenObjC/debug-info-ivars.m
@@ -21,24 +21,24 @@
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "i"
 // CHECK-SAME:   line: 10
 // CHECK-SAME:   baseType: ![[INT:[0-9]+]]
-// CHECK-SAME:   size: 32, align: 32,
+// CHECK-SAME:   size: 32,
 // CHECK-NOT:offset:
 // CHECK-SAME:   flags: DIFlagProtected
 // CHECK: ![[INT]] = !DIBasicType(name: "int"
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_1"
 // CHECK-SAME:   line: 11
 // CHECK-SAME:   baseType: ![[UNSIGNED:[0-9]+]]
-// CHECK-SAME:   size: 9, align: 32,
+// CHECK-SAME:   size: 9,
 // CHECK-NOT:offset:
 // CHECK-SAME:   flags: DIFlagProtected
 // CHECK: ![[UNSIGNED]] = !DIBasicType(name: "unsigned int"
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_2"
 // CHECK-SAME:   line: 12
 // CHECK-SAME:   baseType: ![[UNSIGNED]]
-// CHECK-SAME:   size: 9, align: 32, offset: 1,
+// CHECK-SAME:   size: 9, offset: 1,
 // CHECK-SAME:   flags: DIFlagProtected
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_3"
 // CHECK-SAME:   line: 14
 // CHECK-SAME:   baseType: ![[UNSIGNED]]
-// CHECK-SAME:   size: 9, align: 32, offset: 3,
+// CHECK-SAME:   size: 9, offset: 3,
 // CHECK-SAME:   flags: DIFlagProtected
Index: test/CodeGenObjC/debug-info-ivars-private.m
===
--- test/CodeGenObjC/debug-info-ivars-private.m
+++ test/CodeGenObjC/debug-info-ivars-private.m
@@ -35,13 +35,13 @@
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "foo"
 // CHECK-SAME:   line: 14
 // CHECK-SAME:   baseType: ![[INT:[0-9]+]]
-// CHECK-SAME:   size: 32, align: 32,
+// CHECK-SAME:   size: 32,
 // CHECK-NOT:offset:
 // CHECK-SAME:   flags: DIFlagProtected
 // CHECK: ![[INT]] = !DIBasicType(name: "int"
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "bar"
 // CHECK-SAME:   line: 27
 // CHECK-SAME:   baseType: ![[INT:[0-9]+]]
-// CHECK-SAME:   size: 32, align: 32,
+// CHECK-SAME:   size: 32,
 // CHECK-NOT:offset:
 // CHECK-SAME:   flags: DIFlagPrivate
Index: test/CodeGenObjC/debug-info-ivars-extension.m
===
--- test/CodeGenObjC/debug-info-ivars-extension.m
+++ test/CodeGenObjC/debug-info-ivars-extension.m
@@ -30,7 +30,7 @@
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "a"
 // CHECK-SAME:   line: 7
 // CHECK-SAME:   baseType: ![[INT:[0-9]+]]
-// CHECK-SAME:   size: 32, align: 32
+// CHECK-SAME:   size: 32
 // CHECK-NOT:offset:
 // CHECK-SAME:   flags: DIFlagPublic
 // CHECK: ![[INT]] = !DIBasi

[PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced

2016-10-03 Thread Reid Kleckner via cfe-commits 
rnk accepted this revision.
rnk added a reviewer: rnk.
rnk added a comment.
This revision is now accepted and ready to land.

lgtm



> CGDebugInfo.cpp:54-56
> +  auto TI = Ctx.getTypeInfo(Ty);
> +  return TI.AlignIsRequired ? Ctx.toCharUnitsFromBits(TI.Align).getQuantity()
> +: 0;

Don't duplicate, just delegate to the Type * overload:

  return getTypeAlignIfRequired(Ty.getTypePtr(), Ctx);

https://reviews.llvm.org/D24426



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced

2016-10-04 Thread Victor Leschuk via cfe-commits 
vleschuk updated this revision to Diff 73537.
vleschuk added a comment.

Pass alignment in bits instead of bytes to backend (conversion is done when 
emitting DW_AT_alignment).


https://reviews.llvm.org/D24426

Files:
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/CGDebugInfo.h
  test/CodeGen/debug-info-packed-struct.c
  test/CodeGen/debug-info-vector.c
  test/CodeGenCXX/debug-info-calling-conventions.cpp
  test/CodeGenCXX/debug-info-enum-class.cpp
  test/CodeGenCXX/debug-info-indirect-field-decl.cpp
  test/CodeGenCXX/debug-info-ms-bitfields.cpp
  test/CodeGenCXX/debug-info-ms-ptr-to-member.cpp
  test/CodeGenCXX/debug-info-rvalue-ref.cpp
  test/CodeGenCXX/debug-info-template-quals.cpp
  test/CodeGenCXX/debug-info-template.cpp
  test/CodeGenCXX/debug-info-union.cpp
  test/CodeGenCXX/debug-info-uuid.cpp
  test/CodeGenCXX/debug-info-vla.cpp
  test/CodeGenCXX/debug-info-zero-length-arrays.cpp
  test/CodeGenCXX/debug-info.cpp
  test/CodeGenCXX/debug-lambda-this.cpp
  test/CodeGenObjC/block-byref-debuginfo.m
  test/CodeGenObjC/debug-info-block-type.m
  test/CodeGenObjC/debug-info-ivars-extension.m
  test/CodeGenObjC/debug-info-ivars-private.m
  test/CodeGenObjC/debug-info-ivars.m
  test/CodeGenObjCXX/debug-info-cyclic.mm

Index: test/CodeGenObjCXX/debug-info-cyclic.mm
===
--- test/CodeGenObjCXX/debug-info-cyclic.mm
+++ test/CodeGenObjCXX/debug-info-cyclic.mm
@@ -3,7 +3,7 @@
 struct B {
 // CHECK: ![[B:[0-9]+]] ={{.*}}!DICompositeType(tag: DW_TAG_structure_type, name: "B"
 // CHECK-SAME: line: [[@LINE-2]],
-// CHECK-SAME: size: 8, align: 8,
+// CHECK-SAME: size: 8,
 // CHECK-NOT:  offset:
 // CHECK-NOT:  DIFlagFwdDecl
 // CHECK-SAME: elements: ![[BMEMBERS:[0-9]+]]
Index: test/CodeGenObjC/debug-info-ivars.m
===
--- test/CodeGenObjC/debug-info-ivars.m
+++ test/CodeGenObjC/debug-info-ivars.m
@@ -21,24 +21,24 @@
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "i"
 // CHECK-SAME:   line: 10
 // CHECK-SAME:   baseType: ![[INT:[0-9]+]]
-// CHECK-SAME:   size: 32, align: 32,
+// CHECK-SAME:   size: 32,
 // CHECK-NOT:offset:
 // CHECK-SAME:   flags: DIFlagProtected
 // CHECK: ![[INT]] = !DIBasicType(name: "int"
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_1"
 // CHECK-SAME:   line: 11
 // CHECK-SAME:   baseType: ![[UNSIGNED:[0-9]+]]
-// CHECK-SAME:   size: 9, align: 32,
+// CHECK-SAME:   size: 9,
 // CHECK-NOT:offset:
 // CHECK-SAME:   flags: DIFlagProtected
 // CHECK: ![[UNSIGNED]] = !DIBasicType(name: "unsigned int"
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_2"
 // CHECK-SAME:   line: 12
 // CHECK-SAME:   baseType: ![[UNSIGNED]]
-// CHECK-SAME:   size: 9, align: 32, offset: 1,
+// CHECK-SAME:   size: 9, offset: 1,
 // CHECK-SAME:   flags: DIFlagProtected
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_3"
 // CHECK-SAME:   line: 14
 // CHECK-SAME:   baseType: ![[UNSIGNED]]
-// CHECK-SAME:   size: 9, align: 32, offset: 3,
+// CHECK-SAME:   size: 9, offset: 3,
 // CHECK-SAME:   flags: DIFlagProtected
Index: test/CodeGenObjC/debug-info-ivars-private.m
===
--- test/CodeGenObjC/debug-info-ivars-private.m
+++ test/CodeGenObjC/debug-info-ivars-private.m
@@ -35,13 +35,13 @@
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "foo"
 // CHECK-SAME:   line: 14
 // CHECK-SAME:   baseType: ![[INT:[0-9]+]]
-// CHECK-SAME:   size: 32, align: 32,
+// CHECK-SAME:   size: 32,
 // CHECK-NOT:offset:
 // CHECK-SAME:   flags: DIFlagProtected
 // CHECK: ![[INT]] = !DIBasicType(name: "int"
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "bar"
 // CHECK-SAME:   line: 27
 // CHECK-SAME:   baseType: ![[INT:[0-9]+]]
-// CHECK-SAME:   size: 32, align: 32,
+// CHECK-SAME:   size: 32,
 // CHECK-NOT:offset:
 // CHECK-SAME:   flags: DIFlagPrivate
Index: test/CodeGenObjC/debug-info-ivars-extension.m
===
--- test/CodeGenObjC/debug-info-ivars-extension.m
+++ test/CodeGenObjC/debug-info-ivars-extension.m
@@ -30,7 +30,7 @@
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "a"
 // CHECK-SAME:   line: 7
 // CHECK-SAME:   baseType: ![[INT:[0-9]+]]
-// CHECK-SAME:   size: 32, align: 32
+// CHECK-SAME:   size: 32
 // CHECK-NOT:offset:
 // CHECK-SAME:   flags: DIFlagPublic
 // CHECK: ![[INT]] = !DIBasicType(name: "int"
@@ -42,6 +42,6 @@
 // CHECK: !DIDerivedType(tag: DW

[PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced

2016-10-14 Thread Victor Leschuk via cfe-commits 
vleschuk updated this revision to Diff 74686.
vleschuk added a comment.

Use DIAlignment type instead of uint64_t for alignment in DebugInfo.


https://reviews.llvm.org/D24426

Files:
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/CGDebugInfo.h
  test/CodeGen/debug-info-packed-struct.c
  test/CodeGen/debug-info-vector.c
  test/CodeGenCXX/debug-info-calling-conventions.cpp
  test/CodeGenCXX/debug-info-enum-class.cpp
  test/CodeGenCXX/debug-info-indirect-field-decl.cpp
  test/CodeGenCXX/debug-info-ms-bitfields.cpp
  test/CodeGenCXX/debug-info-ms-ptr-to-member.cpp
  test/CodeGenCXX/debug-info-rvalue-ref.cpp
  test/CodeGenCXX/debug-info-template-quals.cpp
  test/CodeGenCXX/debug-info-template.cpp
  test/CodeGenCXX/debug-info-union.cpp
  test/CodeGenCXX/debug-info-uuid.cpp
  test/CodeGenCXX/debug-info-vla.cpp
  test/CodeGenCXX/debug-info-zero-length-arrays.cpp
  test/CodeGenCXX/debug-info.cpp
  test/CodeGenCXX/debug-lambda-this.cpp
  test/CodeGenObjC/block-byref-debuginfo.m
  test/CodeGenObjC/debug-info-block-type.m
  test/CodeGenObjC/debug-info-ivars-extension.m
  test/CodeGenObjC/debug-info-ivars-private.m
  test/CodeGenObjC/debug-info-ivars.m
  test/CodeGenObjCXX/debug-info-cyclic.mm

Index: test/CodeGenObjCXX/debug-info-cyclic.mm
===
--- test/CodeGenObjCXX/debug-info-cyclic.mm
+++ test/CodeGenObjCXX/debug-info-cyclic.mm
@@ -3,7 +3,7 @@
 struct B {
 // CHECK: ![[B:[0-9]+]] ={{.*}}!DICompositeType(tag: DW_TAG_structure_type, name: "B"
 // CHECK-SAME: line: [[@LINE-2]],
-// CHECK-SAME: size: 8, align: 8,
+// CHECK-SAME: size: 8,
 // CHECK-NOT:  offset:
 // CHECK-NOT:  DIFlagFwdDecl
 // CHECK-SAME: elements: ![[BMEMBERS:[0-9]+]]
Index: test/CodeGenObjC/debug-info-ivars.m
===
--- test/CodeGenObjC/debug-info-ivars.m
+++ test/CodeGenObjC/debug-info-ivars.m
@@ -21,24 +21,24 @@
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "i"
 // CHECK-SAME:   line: 10
 // CHECK-SAME:   baseType: ![[INT:[0-9]+]]
-// CHECK-SAME:   size: 32, align: 32,
+// CHECK-SAME:   size: 32,
 // CHECK-NOT:offset:
 // CHECK-SAME:   flags: DIFlagProtected
 // CHECK: ![[INT]] = !DIBasicType(name: "int"
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_1"
 // CHECK-SAME:   line: 11
 // CHECK-SAME:   baseType: ![[UNSIGNED:[0-9]+]]
-// CHECK-SAME:   size: 9, align: 32,
+// CHECK-SAME:   size: 9,
 // CHECK-NOT:offset:
 // CHECK-SAME:   flags: DIFlagProtected
 // CHECK: ![[UNSIGNED]] = !DIBasicType(name: "unsigned int"
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_2"
 // CHECK-SAME:   line: 12
 // CHECK-SAME:   baseType: ![[UNSIGNED]]
-// CHECK-SAME:   size: 9, align: 32, offset: 1,
+// CHECK-SAME:   size: 9, offset: 1,
 // CHECK-SAME:   flags: DIFlagProtected
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_3"
 // CHECK-SAME:   line: 14
 // CHECK-SAME:   baseType: ![[UNSIGNED]]
-// CHECK-SAME:   size: 9, align: 32, offset: 3,
+// CHECK-SAME:   size: 9, offset: 3,
 // CHECK-SAME:   flags: DIFlagProtected
Index: test/CodeGenObjC/debug-info-ivars-private.m
===
--- test/CodeGenObjC/debug-info-ivars-private.m
+++ test/CodeGenObjC/debug-info-ivars-private.m
@@ -35,13 +35,13 @@
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "foo"
 // CHECK-SAME:   line: 14
 // CHECK-SAME:   baseType: ![[INT:[0-9]+]]
-// CHECK-SAME:   size: 32, align: 32,
+// CHECK-SAME:   size: 32,
 // CHECK-NOT:offset:
 // CHECK-SAME:   flags: DIFlagProtected
 // CHECK: ![[INT]] = !DIBasicType(name: "int"
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "bar"
 // CHECK-SAME:   line: 27
 // CHECK-SAME:   baseType: ![[INT:[0-9]+]]
-// CHECK-SAME:   size: 32, align: 32,
+// CHECK-SAME:   size: 32,
 // CHECK-NOT:offset:
 // CHECK-SAME:   flags: DIFlagPrivate
Index: test/CodeGenObjC/debug-info-ivars-extension.m
===
--- test/CodeGenObjC/debug-info-ivars-extension.m
+++ test/CodeGenObjC/debug-info-ivars-extension.m
@@ -30,7 +30,7 @@
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "a"
 // CHECK-SAME:   line: 7
 // CHECK-SAME:   baseType: ![[INT:[0-9]+]]
-// CHECK-SAME:   size: 32, align: 32
+// CHECK-SAME:   size: 32
 // CHECK-NOT:offset:
 // CHECK-SAME:   flags: DIFlagPublic
 // CHECK: ![[INT]] = !DIBasicType(name: "int"
@@ -42,6 +42,6 @@
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "b"
 // CHECK-SA

Re: [PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced

2016-09-11 Thread Victor Leschuk via cfe-commits 
vleschuk retitled this revision from "DebugInfo: use 
llvm::DINode::FlagAlignment to mark forcibly aligned data" to "DebugInfo: Pass 
non-zero alignment to DIBuilder only if aligment was forced".
vleschuk updated the summary for this revision.
vleschuk updated this revision to Diff 70965.
vleschuk added a comment.

Got rid of DINode::FlagAlignment. Check for forced alignment (alignas(), 
_Alignas(), __attribute(aligned (N) when creating DI* objects with 
DIBuilder. If alignment was not forced pass zero, otherwise pass actual 
alignment value. Updated related tests according to new behavior.


https://reviews.llvm.org/D24426

Files:
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/CGDebugInfo.h
  test/CodeGen/debug-info-packed-struct.c
  test/CodeGen/debug-info-vector.c
  test/CodeGenCXX/debug-info-calling-conventions.cpp
  test/CodeGenCXX/debug-info-enum-class.cpp
  test/CodeGenCXX/debug-info-indirect-field-decl.cpp
  test/CodeGenCXX/debug-info-ms-bitfields.cpp
  test/CodeGenCXX/debug-info-ms-ptr-to-member.cpp
  test/CodeGenCXX/debug-info-rvalue-ref.cpp
  test/CodeGenCXX/debug-info-template-quals.cpp
  test/CodeGenCXX/debug-info-template.cpp
  test/CodeGenCXX/debug-info-union.cpp
  test/CodeGenCXX/debug-info-uuid.cpp
  test/CodeGenCXX/debug-info-vla.cpp
  test/CodeGenCXX/debug-info-zero-length-arrays.cpp
  test/CodeGenCXX/debug-info.cpp
  test/CodeGenCXX/debug-lambda-this.cpp
  test/CodeGenObjC/block-byref-debuginfo.m
  test/CodeGenObjC/debug-info-block-type.m
  test/CodeGenObjC/debug-info-ivars-extension.m
  test/CodeGenObjC/debug-info-ivars-private.m
  test/CodeGenObjC/debug-info-ivars.m
  test/CodeGenObjCXX/debug-info-cyclic.mm

Index: test/CodeGenObjCXX/debug-info-cyclic.mm
===
--- test/CodeGenObjCXX/debug-info-cyclic.mm
+++ test/CodeGenObjCXX/debug-info-cyclic.mm
@@ -3,7 +3,7 @@
 struct B {
 // CHECK: ![[B:[0-9]+]] ={{.*}}!DICompositeType(tag: DW_TAG_structure_type, name: "B"
 // CHECK-SAME: line: [[@LINE-2]],
-// CHECK-SAME: size: 8, align: 8,
+// CHECK-SAME: size: 8,
 // CHECK-NOT:  offset:
 // CHECK-NOT:  DIFlagFwdDecl
 // CHECK-SAME: elements: ![[BMEMBERS:[0-9]+]]
Index: test/CodeGenObjC/debug-info-ivars.m
===
--- test/CodeGenObjC/debug-info-ivars.m
+++ test/CodeGenObjC/debug-info-ivars.m
@@ -21,24 +21,24 @@
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "i"
 // CHECK-SAME:   line: 10
 // CHECK-SAME:   baseType: ![[INT:[0-9]+]]
-// CHECK-SAME:   size: 32, align: 32,
+// CHECK-SAME:   size: 32,
 // CHECK-NOT:offset:
 // CHECK-SAME:   flags: DIFlagProtected
 // CHECK: ![[INT]] = !DIBasicType(name: "int"
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_1"
 // CHECK-SAME:   line: 11
 // CHECK-SAME:   baseType: ![[UNSIGNED:[0-9]+]]
-// CHECK-SAME:   size: 9, align: 32,
+// CHECK-SAME:   size: 9,
 // CHECK-NOT:offset:
 // CHECK-SAME:   flags: DIFlagProtected
 // CHECK: ![[UNSIGNED]] = !DIBasicType(name: "unsigned int"
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_2"
 // CHECK-SAME:   line: 12
 // CHECK-SAME:   baseType: ![[UNSIGNED]]
-// CHECK-SAME:   size: 9, align: 32, offset: 1,
+// CHECK-SAME:   size: 9, offset: 1,
 // CHECK-SAME:   flags: DIFlagProtected
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_3"
 // CHECK-SAME:   line: 14
 // CHECK-SAME:   baseType: ![[UNSIGNED]]
-// CHECK-SAME:   size: 9, align: 32, offset: 3,
+// CHECK-SAME:   size: 9, offset: 3,
 // CHECK-SAME:   flags: DIFlagProtected
Index: test/CodeGenObjC/debug-info-ivars-private.m
===
--- test/CodeGenObjC/debug-info-ivars-private.m
+++ test/CodeGenObjC/debug-info-ivars-private.m
@@ -35,13 +35,13 @@
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "foo"
 // CHECK-SAME:   line: 14
 // CHECK-SAME:   baseType: ![[INT:[0-9]+]]
-// CHECK-SAME:   size: 32, align: 32,
+// CHECK-SAME:   size: 32,
 // CHECK-NOT:offset:
 // CHECK-SAME:   flags: DIFlagProtected
 // CHECK: ![[INT]] = !DIBasicType(name: "int"
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "bar"
 // CHECK-SAME:   line: 27
 // CHECK-SAME:   baseType: ![[INT:[0-9]+]]
-// CHECK-SAME:   size: 32, align: 32,
+// CHECK-SAME:   size: 32,
 // CHECK-NOT:offset:
 // CHECK-SAME:   flags: DIFlagPrivate
Index: test/CodeGenObjC/debug-info-ivars-extension.m
===
--- test/CodeGenObjC/debug-info-ivars-extension.m
+++ test/CodeGenObjC/debug-info-ivars-extens

Re: [PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced

2016-09-12 Thread David Blaikie via cfe-commits 
dblaikie added a comment.

Like the backend patch, should/could this be broken up into separate patches 
for the different places/features that have alignment?

(probably just passing zero for alignment in general could be a simple cleanup 
patch to start, if it's otherwise unused)



Comment at: lib/CodeGen/CGDebugInfo.cpp:609
@@ -609,3 +608,3 @@
   uint64_t Size = CGM.getContext().getTypeSize(Ty);
-  uint64_t Align = CGM.getContext().getTypeAlign(Ty);
+  uint64_t Align = 0;
   return DBuilder.createBasicType("complex", Size, Align, Encoding);

Maybe add a comment here about why we're specifying zero alignment (or possibly 
change the API to not take an alignment at all so we don't have to explain it?)

(similarly for other cases where the alignment is just hardcoded to zero)


Comment at: lib/CodeGen/CGDebugInfo.cpp:3691
@@ -3635,1 +3690,3 @@
+  if (D->hasAttr())
+AlignInBits = D->getMaxAlignment();
   StringRef DeclName, LinkageName;

is max alignment the right thing here? Should it be min alignment?
(is alignment in bits the desired thing across all of this too? It looked like 
in the backend patch there was some division by CHAR_BITS, etc?)


https://reviews.llvm.org/D24426



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced

2016-09-12 Thread Paul Robinson via cfe-commits 
probinson added a subscriber: probinson.


Comment at: lib/CodeGen/CGDebugInfo.cpp:3691
@@ -3635,1 +3690,3 @@
+  if (D->hasAttr())
+AlignInBits = D->getMaxAlignment();
   StringRef DeclName, LinkageName;

dblaikie wrote:
> is max alignment the right thing here? Should it be min alignment?
> (is alignment in bits the desired thing across all of this too? It looked 
> like in the backend patch there was some division by CHAR_BITS, etc?)
I should think bits is the right choice here; seems more the province of the 
backend to convert it into the appropriate addressable units (commonly but not 
universally chars).


https://reviews.llvm.org/D24426



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced

2016-09-12 Thread David Blaikie via cfe-commits 
On Mon, Sep 12, 2016 at 5:00 PM Paul Robinson 
wrote:

> probinson added a subscriber: probinson.
>
> 
> Comment at: lib/CodeGen/CGDebugInfo.cpp:3691
> @@ -3635,1 +3690,3 @@
> +  if (D->hasAttr())
> +AlignInBits = D->getMaxAlignment();
>StringRef DeclName, LinkageName;
> 
> dblaikie wrote:
> > is max alignment the right thing here? Should it be min alignment?
> > (is alignment in bits the desired thing across all of this too? It
> looked like in the backend patch there was some division by CHAR_BITS, etc?)
> I should think bits is the right choice here; seems more the province of
> the backend to convert it into the appropriate addressable units (commonly
> but not universally chars).
>

The alternative thinking is that we've a generally sense we want to make
more of this type information opaque to LLVM - so I'm somewhat inclined to
make the frontend do the work of choosing what to emit and the backend just
being as simple as possible.

Hmm, seems like the DWARF spec details I can find:
http://www.dwarfstd.org/ShowIssue.php?issue=140528.1 don't really specify
what the value of DW_AT_alignment is, it's sort of assumed, by the looks of
it? I'm assuming it's bytes, the same as the byte_size attribute.




>
>
> https://reviews.llvm.org/D24426
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced

2016-09-12 Thread Reid Kleckner via cfe-commits 
rnk added a subscriber: rnk.


Comment at: lib/CodeGen/CGDebugInfo.cpp:608
@@ -608,2 +607,3 @@
 
   uint64_t Size = CGM.getContext().getTypeSize(Ty);
+  uint64_t Align = 0;

IMO this is what we should be doing everywhere, rather than manually checking 
AlignedAttr:
  TypeInfo TI = CGM.getContext().getTypeInfo(Ty);
  uint64_t Size = TI.Width;
  uint64_t Align = TI.AlignIsRequired ? TI.Align : 0;

This saves a hash lookup, and handles some corner cases. AlignIsRequired is 
already supposed to capture whether the alignment was changed.


https://reviews.llvm.org/D24426



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

RE: [PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced

2016-09-12 Thread Robinson, Paul via cfe-commits 
The text in the committee draft is different (e.g., the exhortation about 
non-default alignment is gone), with an example to the effect that a value of 8 
means the entity's address is a multiple of 8 (not 2^8).  So, alignment is 
conceived in terms of address bits, whatever those represent (not always bytes).

If Clang is being infested with more target knowledge, okay, but that means 
tolerating the weirder targets in these cases.  Dividing by CHAR_BITS makes an 
assumption that isn't necessarily correct.
--paulr
P.S. The committee is hoping to get a draft out for public comment Real Soon 
Now.

From: cfe-commits [mailto:cfe-commits-boun...@lists.llvm.org] On Behalf Of 
David Blaikie via cfe-commits
Sent: Monday, September 12, 2016 5:12 PM
To: reviews+d24426+public+6ee6274d38fdf...@reviews.llvm.org; 
vlesc...@accesssoftek.com; echri...@gmail.com; apra...@apple.com; 
mehdi.am...@apple.com
Cc: cfe-commits@lists.llvm.org
Subject: Re: [PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder 
only if aligment was forced


On Mon, Sep 12, 2016 at 5:00 PM Paul Robinson 
mailto:paul.robin...@sony.com>> wrote:
probinson added a subscriber: probinson.


Comment at: lib/CodeGen/CGDebugInfo.cpp:3691
@@ -3635,1 +3690,3 @@
+  if (D->hasAttr())
+AlignInBits = D->getMaxAlignment();
   StringRef DeclName, LinkageName;

dblaikie wrote:
> is max alignment the right thing here? Should it be min alignment?
> (is alignment in bits the desired thing across all of this too? It looked 
> like in the backend patch there was some division by CHAR_BITS, etc?)
I should think bits is the right choice here; seems more the province of the 
backend to convert it into the appropriate addressable units (commonly but not 
universally chars).

The alternative thinking is that we've a generally sense we want to make more 
of this type information opaque to LLVM - so I'm somewhat inclined to make the 
frontend do the work of choosing what to emit and the backend just being as 
simple as possible.

Hmm, seems like the DWARF spec details I can find: 
http://www.dwarfstd.org/ShowIssue.php?issue=140528.1 don't really specify what 
the value of DW_AT_alignment is, it's sort of assumed, by the looks of it? I'm 
assuming it's bytes, the same as the byte_size attribute.





https://reviews.llvm.org/D24426


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced

2016-09-12 Thread David Blaikie via cfe-commits 
On Mon, Sep 12, 2016 at 6:01 PM Robinson, Paul 
wrote:

> The text in the committee draft is different (e.g., the exhortation about
> non-default alignment is gone), with an example to the effect that a value
> of 8 means the entity's address is a multiple of 8 (not 2^8).  So,
> alignment is conceived in terms of address bits, whatever those represent
> (not always bytes).
>
Not sure I quite follow. OK, so in an octet addressable world (which LLVM
is - there have been some attempts to support non-octet addressing, but I
don't think any have been near to successful) then DW_AT_alignment is byte
alignment (1 means there are no zero bits in the address, 2 means there's 1
trailing zero bit in the address, etc).

> If Clang is being infested with more target knowledge, okay, but that
> means tolerating the weirder targets in these cases.  Dividing by CHAR_BITS
> makes an assumption that isn't necessarily correct.
>
Clang has the knowledge already - it knows the alignment of the types its
allocating, etc. So I'm not sure what infestation you're referring to.

I've sort of lost track of what we're discussing here.

Essentially what I'm suggesting is that Clang should put whatever number is
going to go in the DWARF, into the metadata. I don't believe the LLVM
backends have greater knowledge than the frontend does in this domain -
have I missed something there, are there examples where that could/would be
true?

- David


> --paulr
>
> P.S. The committee is hoping to get a draft out for public comment Real
> Soon Now.
>
Looking forward to it :)

>
>
> *From:* cfe-commits [mailto:cfe-commits-boun...@lists.llvm.org] *On
> Behalf Of *David Blaikie via cfe-commits
> *Sent:* Monday, September 12, 2016 5:12 PM
> *To:* reviews+d24426+public+6ee6274d38fdf...@reviews.llvm.org;
> vlesc...@accesssoftek.com; echri...@gmail.com; apra...@apple.com;
> mehdi.am...@apple.com
> *Cc:* cfe-commits@lists.llvm.org
> *Subject:* Re: [PATCH] D24426: DebugInfo: Pass non-zero alignment to
> DIBuilder only if aligment was forced
>
>
>
>
>
> On Mon, Sep 12, 2016 at 5:00 PM Paul Robinson 
> wrote:
>
> probinson added a subscriber: probinson.
>
> 
> Comment at: lib/CodeGen/CGDebugInfo.cpp:3691
> @@ -3635,1 +3690,3 @@
> +  if (D->hasAttr())
> +AlignInBits = D->getMaxAlignment();
>StringRef DeclName, LinkageName;
> 
> dblaikie wrote:
> > is max alignment the right thing here? Should it be min alignment?
> > (is alignment in bits the desired thing across all of this too? It
> looked like in the backend patch there was some division by CHAR_BITS, etc?)
> I should think bits is the right choice here; seems more the province of
> the backend to convert it into the appropriate addressable units (commonly
> but not universally chars).
>
>
> The alternative thinking is that we've a generally sense we want to make
> more of this type information opaque to LLVM - so I'm somewhat inclined to
> make the frontend do the work of choosing what to emit and the backend just
> being as simple as possible.
>
> Hmm, seems like the DWARF spec details I can find:
> http://www.dwarfstd.org/ShowIssue.php?issue=140528.1 don't really specify
> what the value of DW_AT_alignment is, it's sort of assumed, by the looks of
> it? I'm assuming it's bytes, the same as the byte_size attribute.
>
>
>
>
>
>
> https://reviews.llvm.org/D24426
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

RE: [PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced

2016-09-13 Thread Robinson, Paul via cfe-commits 
I hadn't thought Clang wanted to be *quite* so knowledgeable about targets, and 
similarly not so tightly tied to byte-addressable targets.  But if both of 
those things are actually okay, then it's fine to set the alignment value here 
to what would be passed through to DWARF.
--paulr

From: David Blaikie [mailto:dblai...@gmail.com]
Sent: Monday, September 12, 2016 6:11 PM
To: Robinson, Paul; reviews+d24426+public+6ee6274d38fdf...@reviews.llvm.org; 
vlesc...@accesssoftek.com; echri...@gmail.com; apra...@apple.com; 
mehdi.am...@apple.com
Cc: cfe-commits (cfe-commits@lists.llvm.org)
Subject: Re: [PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder 
only if aligment was forced


On Mon, Sep 12, 2016 at 6:01 PM Robinson, Paul 
mailto:paul.robin...@sony.com>> wrote:
The text in the committee draft is different (e.g., the exhortation about 
non-default alignment is gone), with an example to the effect that a value of 8 
means the entity's address is a multiple of 8 (not 2^8).  So, alignment is 
conceived in terms of address bits, whatever those represent (not always bytes).
Not sure I quite follow. OK, so in an octet addressable world (which LLVM is - 
there have been some attempts to support non-octet addressing, but I don't 
think any have been near to successful) then DW_AT_alignment is byte alignment 
(1 means there are no zero bits in the address, 2 means there's 1 trailing zero 
bit in the address, etc).
If Clang is being infested with more target knowledge, okay, but that means 
tolerating the weirder targets in these cases.  Dividing by CHAR_BITS makes an 
assumption that isn't necessarily correct.
Clang has the knowledge already - it knows the alignment of the types its 
allocating, etc. So I'm not sure what infestation you're referring to.

I've sort of lost track of what we're discussing here.

Essentially what I'm suggesting is that Clang should put whatever number is 
going to go in the DWARF, into the metadata. I don't believe the LLVM backends 
have greater knowledge than the frontend does in this domain - have I missed 
something there, are there examples where that could/would be true?

- David

--paulr
P.S. The committee is hoping to get a draft out for public comment Real Soon 
Now.
Looking forward to it :)

From: cfe-commits 
[mailto:cfe-commits-boun...@lists.llvm.org<mailto:cfe-commits-boun...@lists.llvm.org>]
 On Behalf Of David Blaikie via cfe-commits
Sent: Monday, September 12, 2016 5:12 PM
To: 
reviews+d24426+public+6ee6274d38fdf...@reviews.llvm.org<mailto:reviews%2bd24426%2bpublic%2b6ee6274d38fdf...@reviews.llvm.org>;
 vlesc...@accesssoftek.com<mailto:vlesc...@accesssoftek.com>; 
echri...@gmail.com<mailto:echri...@gmail.com>; 
apra...@apple.com<mailto:apra...@apple.com>; 
mehdi.am...@apple.com<mailto:mehdi.am...@apple.com>
Cc: cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>
Subject: Re: [PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder 
only if aligment was forced


On Mon, Sep 12, 2016 at 5:00 PM Paul Robinson 
mailto:paul.robin...@sony.com>> wrote:
probinson added a subscriber: probinson.


Comment at: lib/CodeGen/CGDebugInfo.cpp:3691
@@ -3635,1 +3690,3 @@
+  if (D->hasAttr())
+AlignInBits = D->getMaxAlignment();
   StringRef DeclName, LinkageName;

dblaikie wrote:
> is max alignment the right thing here? Should it be min alignment?
> (is alignment in bits the desired thing across all of this too? It looked 
> like in the backend patch there was some division by CHAR_BITS, etc?)
I should think bits is the right choice here; seems more the province of the 
backend to convert it into the appropriate addressable units (commonly but not 
universally chars).

The alternative thinking is that we've a generally sense we want to make more 
of this type information opaque to LLVM - so I'm somewhat inclined to make the 
frontend do the work of choosing what to emit and the backend just being as 
simple as possible.

Hmm, seems like the DWARF spec details I can find: 
http://www.dwarfstd.org/ShowIssue.php?issue=140528.1 don't really specify what 
the value of DW_AT_alignment is, it's sort of assumed, by the looks of it? I'm 
assuming it's bytes, the same as the byte_size attribute.





https://reviews.llvm.org/D24426

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced

2016-09-13 Thread Victor Leschuk via cfe-commits 
vleschuk added inline comments.


Comment at: lib/CodeGen/CGDebugInfo.cpp:3691
@@ -3635,1 +3690,3 @@
+  if (D->hasAttr())
+AlignInBits = D->getMaxAlignment();
   StringRef DeclName, LinkageName;

probinson wrote:
> dblaikie wrote:
> > is max alignment the right thing here? Should it be min alignment?
> > (is alignment in bits the desired thing across all of this too? It looked 
> > like in the backend patch there was some division by CHAR_BITS, etc?)
> I should think bits is the right choice here; seems more the province of the 
> backend to convert it into the appropriate addressable units (commonly but 
> not universally chars).
> is max alignment the right thing here? Should it be min alignment?

The DWARF document says: 

> This constant describes the actual alignment used by the compiler (if there 
> are multiple specified by the user, or if the user specified an alignment the 
> compiler couldn't satisfy, then only the strictest alignment will be added 
> through a DW_AT_alignment).

So I think max alignment is right choice here.


https://reviews.llvm.org/D24426



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced

2016-09-13 Thread Victor Leschuk via cfe-commits 
vleschuk added inline comments.


Comment at: lib/CodeGen/CGDebugInfo.cpp:608
@@ -608,2 +607,3 @@
 
   uint64_t Size = CGM.getContext().getTypeSize(Ty);
+  uint64_t Align = 0;

rnk wrote:
> IMO this is what we should be doing everywhere, rather than manually checking 
> AlignedAttr:
>   TypeInfo TI = CGM.getContext().getTypeInfo(Ty);
>   uint64_t Size = TI.Width;
>   uint64_t Align = TI.AlignIsRequired ? TI.Align : 0;
> 
> This saves a hash lookup, and handles some corner cases. AlignIsRequired is 
> already supposed to capture whether the alignment was changed.
Will check if this works in all cases. I think it's worth putting this snippet 
into helper function within anon namespace.


https://reviews.llvm.org/D24426



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced

2016-09-13 Thread Victor Leschuk via cfe-commits 
I think it's better to pass amount in bytes here, as specified by user 
(alignas takes bytes, not bits).

On 09/13/2016 09:00 PM, Robinson, Paul wrote:


I hadn't thought Clang wanted to be *quite* so knowledgeable about 
targets, and similarly not so tightly tied to byte-addressable 
targets.  But if both of those things are actually okay, then it's 
fine to set the alignment value here to what would be passed through 
to DWARF.


--paulr

*From:*David Blaikie [mailto:dblai...@gmail.com]
*Sent:* Monday, September 12, 2016 6:11 PM
*To:* Robinson, Paul; 
reviews+d24426+public+6ee6274d38fdf...@reviews.llvm.org; 
vlesc...@accesssoftek.com; echri...@gmail.com; apra...@apple.com; 
mehdi.am...@apple.com

*Cc:* cfe-commits (cfe-commits@lists.llvm.org)
*Subject:* Re: [PATCH] D24426: DebugInfo: Pass non-zero alignment to 
DIBuilder only if aligment was forced


On Mon, Sep 12, 2016 at 6:01 PM Robinson, Paul <mailto:paul.robin...@sony.com>> wrote:


The text in the committee draft is different (e.g., the
exhortation about non-default alignment is gone), with an example
to the effect that a value of 8 means the entity's address is a
multiple of 8 (not 2^8).  So, alignment is conceived in terms of
address bits, whatever those represent (not always bytes).

Not sure I quite follow. OK, so in an octet addressable world (which 
LLVM is - there have been some attempts to support non-octet 
addressing, but I don't think any have been near to successful) then 
DW_AT_alignment is byte alignment (1 means there are no zero bits in 
the address, 2 means there's 1 trailing zero bit in the address, etc).


If Clang is being infested with more target knowledge, okay, but
that means tolerating the weirder targets in these cases. 
Dividing by CHAR_BITS makes an assumption that isn't necessarily

correct.

Clang has the knowledge already - it knows the alignment of the types 
its allocating, etc. So I'm not sure what infestation you're referring to.


I've sort of lost track of what we're discussing here.

Essentially what I'm suggesting is that Clang should put whatever 
number is going to go in the DWARF, into the metadata. I don't believe 
the LLVM backends have greater knowledge than the frontend does in 
this domain - have I missed something there, are there examples where 
that could/would be true?


- David

--paulr

P.S. The committee is hoping to get a draft out for public comment
Real Soon Now.

Looking forward to it :)

*From:*cfe-commits [mailto:cfe-commits-boun...@lists.llvm.org
<mailto:cfe-commits-boun...@lists.llvm.org>] *On Behalf Of *David
Blaikie via cfe-commits
*Sent:* Monday, September 12, 2016 5:12 PM
*To:* reviews+d24426+public+6ee6274d38fdf...@reviews.llvm.org
<mailto:reviews%2bd24426%2bpublic%2b6ee6274d38fdf...@reviews.llvm.org>;
vlesc...@accesssoftek.com <mailto:vlesc...@accesssoftek.com>;
echri...@gmail.com <mailto:echri...@gmail.com>; apra...@apple.com
<mailto:apra...@apple.com>; mehdi.am...@apple.com
<mailto:mehdi.am...@apple.com>
*Cc:* cfe-commits@lists.llvm.org <mailto:cfe-commits@lists.llvm.org>
*Subject:* Re: [PATCH] D24426: DebugInfo: Pass non-zero alignment
to DIBuilder only if aligment was forced

On Mon, Sep 12, 2016 at 5:00 PM Paul Robinson
mailto:paul.robin...@sony.com>> wrote:

probinson added a subscriber: probinson.


Comment at: lib/CodeGen/CGDebugInfo.cpp:3691
@@ -3635,1 +3690,3 @@
+  if (D->hasAttr())
+AlignInBits = D->getMaxAlignment();
   StringRef DeclName, LinkageName;

dblaikie wrote:
> is max alignment the right thing here? Should it be min
alignment?
> (is alignment in bits the desired thing across all of this
too? It looked like in the backend patch there was some
division by CHAR_BITS, etc?)
I should think bits is the right choice here; seems more the
province of the backend to convert it into the appropriate
addressable units (commonly but not universally chars).


The alternative thinking is that we've a generally sense we want
to make more of this type information opaque to LLVM - so I'm
somewhat inclined to make the frontend do the work of choosing
what to emit and the backend just being as simple as possible.

Hmm, seems like the DWARF spec details I can find:
http://www.dwarfstd.org/ShowIssue.php?issue=140528.1 don't really
specify what the value of DW_AT_alignment is, it's sort of
assumed, by the looks of it? I'm assuming it's bytes, the same as
the byte_size attribute.




https://reviews.llvm.org/D24426



--
Best Regards,
Victor

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits