Page MenuHomePhabricator

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

Authored by awpandey on Nov 11 2019, 11:16 PM.

Details

Summary

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

Diff Detail

Event Timeline

awpandey created this revision.Nov 11 2019, 11:16 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 11 2019, 11:16 PM
djtodoro added inline comments.
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"

@awpandey Thanks for the patch!

awpandey updated this revision to Diff 228852.Nov 12 2019, 3:59 AM

Revised and address comments of @djtodoro

awpandey marked 8 inline comments as done.Nov 12 2019, 4:01 AM

Thank you @djtodoro for reviewing this.

aprantl added inline comments.Nov 12 2019, 9:08 AM
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 updated this revision to Diff 229013.Nov 12 2019, 11:02 PM
awpandey marked 3 inline comments as done.

@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 added inline comments.Nov 12 2019, 11:03 PM
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 added inline comments.Nov 14 2019, 11:12 AM
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 updated this revision to Diff 229544.Nov 15 2019, 7:28 AM

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

awpandey marked 2 inline comments as done.Nov 15 2019, 7:28 AM
aprantl accepted this revision.Nov 15 2019, 9:35 AM

LGTM with one change to DIBuilder inline.

llvm/include/llvm/IR/DIBuilder.h
243

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

This revision is now accepted and ready to land.Nov 15 2019, 9:35 AM

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

This revision was automatically updated to reflect the committed changes.

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.

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)

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?

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).

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)

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.

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

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 ??

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.

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

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).

Sounds good to me!

CodaFi added a subscriber: CodaFi.Nov 21 2019, 8:57 AM

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.

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 updated this revision to Diff 231004.Nov 25 2019, 10:50 PM

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

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

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

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

djtodoro added inline comments.Dec 23 2019, 5:56 AM
llvm/docs/ReleaseNotes.rst
169 ↗(On Diff #231004)

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

djtodoro added inline comments.Dec 24 2019, 1:14 AM
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 added inline comments.Dec 25 2019, 11:33 PM
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 added inline comments.Dec 25 2019, 11:40 PM
llvm/docs/ReleaseNotes.rst
169 ↗(On Diff #231004)

The builder is back to passing again.