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

djtodoro added inline comments. Comment at: llvm/docs/ReleaseNotes.rst:169 +* C DebugInfo API ``LLVMDIBuilderCreateTypedef`` is updated to include an extra +argument ``AlignInBits``, to facilitate / propagate specified Alignment information

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

SouraVX added inline comments. Comment at: llvm/docs/ReleaseNotes.rst:169 +* C DebugInfo API ``LLVMDIBuilderCreateTypedef`` is updated to include an extra +argument ``AlignInBits``, to facilitate / propagate specified Alignment information

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

djtodoro added inline comments. Comment at: llvm/docs/ReleaseNotes.rst:169 +* C DebugInfo API ``LLVMDIBuilderCreateTypedef`` is updated to include an extra +argument ``AlignInBits``, to facilitate / propagate specified Alignment information

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

djtodoro added inline comments. Comment at: llvm/docs/ReleaseNotes.rst:169 +* C DebugInfo API ``LLVMDIBuilderCreateTypedef`` is updated to include an extra +argument ``AlignInBits``, to facilitate / propagate specified Alignment information

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

SouraVX added a comment. In D70111#1765223 , @dblaikie wrote: > Looks OK to me - please recommit when you're ready. Thank you @awpandey . As requested, I've recommited this. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70111/new/

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

dblaikie added a comment. Looks OK to me - please recommit when you're ready. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70111/new/ https://reviews.llvm.org/D70111 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

awpandey updated this revision to Diff 231004. awpandey added a comment. Thanks @dblaikie. I have updated go and C bindings. I have also added the release note for why these APIs are changing. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70111/new/ https://reviews.llvm.org/D70111

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

dblaikie added a comment. 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

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

CodaFi added a comment. C binding changes LGTM. Don’t worry about changing them, they are marked as experimental and subject to change at any time. Comment at: llvm/include/llvm-c/DebugInfo.h:878 LLVMMetadataRef File, unsigned LineNo, -

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

aprantl added a comment. Sounds good to me! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70111/new/ https://reviews.llvm.org/D70111 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

awpandey added a comment. 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). Repository: rG LLVM Github Monorepo CHANGES SINCE

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

dblaikie added a comment. Please update the go bindings and include a release note that this API is changing & why. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70111/new/ https://reviews.llvm.org/D70111

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

sammccall added a comment. 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

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

SouraVX added a comment. 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,

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

SouraVX added a comment. 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 Repository: rG LLVM Github Monorepo

Re: [PATCH] D70111: [DWARF5]Addition of alignment field in the typedef for dwarf5

The C bindings, in general, don't have stability guarantees outside of a few cases. Whitequark owns all of this now though :) -eric On Tue, Nov 19, 2019 at 1:38 PM Adrian Prantl via Phabricator wrote: > > aprantl added a subscriber: echristo. > aprantl added a comment. > > In D70111#1752215

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

aprantl added a subscriber: echristo. aprantl added a comment. 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

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

dblaikie added a comment. 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? Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp:804-810 +

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

aprantl added a comment. 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). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

awpandey added a comment. 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? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70111/new/

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

sammccall added a comment. 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) Comment at:

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

This revision was automatically updated to reflect the committed changes. Closed by commit rG423f541c1a32: [DWARF5]Addition of alignment atrribute in typedef DIE. (authored by SouraVX). Changed prior to commit: https://reviews.llvm.org/D70111?vs=229544=229697#toc Repository: rG LLVM Github

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

SouraVX added a comment. 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. Repository: rG LLVM Github

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

awpandey added a comment. Hi @aprantl. I had made the changes and my team member will commit this. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70111/new/ https://reviews.llvm.org/D70111 ___ cfe-commits mailing list

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

aprantl accepted this revision. aprantl added a comment. This revision is now accepted and ready to land. LGTM with one change to DIBuilder inline. Comment at: llvm/include/llvm/IR/DIBuilder.h:243 + unsigned LineNo, DIScope *Context, +

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

awpandey updated this revision to Diff 229544. awpandey added a comment. Thanks for your suggestion @aprantl. I have revised the data type based on your suggestion. Please inform me if any other changes are required. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70111/new/

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

aprantl added inline comments. Comment at: llvm/include/llvm/IR/DIBuilder.h:243 + unsigned LineNo, DIScope *Context, + uint32_t AlignInBits = 0); awpandey wrote: > aprantl wrote: > > This should

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

awpandey added inline comments. Comment at: clang/test/CodeGenCXX/debug-info-template-align.cpp:8 + +// CHECK: DIDerivedType(tag: DW_TAG_typedef, {{.*}}, align: + aprantl wrote: > Do we need to hardcode the target here? Could we check for the specific align >

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

awpandey updated this revision to Diff 229013. awpandey marked 3 inline comments as done. awpandey added a comment. Herald added a reviewer: deadalnix. @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

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

aprantl added inline comments. Comment at: clang/test/CodeGenCXX/debug-info-template-align.cpp:8 + +// CHECK: DIDerivedType(tag: DW_TAG_typedef, {{.*}}, align: + Do we need to hardcode the target here? Could we check for the specific align value?

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

awpandey marked 8 inline comments as done. awpandey added a comment. Thank you @djtodoro for reviewing this. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70111/new/ https://reviews.llvm.org/D70111 ___ cfe-commits mailing list

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

awpandey updated this revision to Diff 228852. awpandey added a comment. Revised and address comments of @djtodoro CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70111/new/ https://reviews.llvm.org/D70111 Files: clang/lib/CodeGen/CGDebugInfo.cpp

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

djtodoro added a comment. @awpandey Thanks for the patch! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70111/new/ https://reviews.llvm.org/D70111 ___ cfe-commits mailing list

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

djtodoro added inline comments. Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp:803 + if (Tag == dwarf::DW_TAG_typedef && DD->getDwarfVersion() >= 5) { +uint32_t AlignInBytes = DTy->getAlignInBytes(); Please add a comment here.

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

awpandey created this revision. awpandey added reviewers: aprantl, dblaikie, jini.susan.george, SouraVX, alok. awpandey added a project: debug-info. Herald added subscribers: llvm-commits, cfe-commits, ormris, hiraditya. Herald added projects: clang, LLVM. This patch, adds support for