Page MenuHomePhabricator

[llvm] Add support for DW_TAG_immutable_type
ClosedPublic

Authored by ljmf00 on Nov 10 2021, 5:23 PM.

Details

Summary

Added documentation about DW_TAG_immutable_type too.

Diff Detail

Event Timeline

ljmf00 created this revision.Nov 10 2021, 5:23 PM
ljmf00 requested review of this revision.Nov 10 2021, 5:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 10 2021, 5:23 PM
ljmf00 planned changes to this revision.Nov 10 2021, 5:24 PM

I still need to add tests

ljmf00 added a subscriber: Geod24.Nov 11 2021, 6:22 AM

Typically I've done "support" for a tag or other DWARF feature in several phases, each usually (not necessarily) with its own patch. Each phase builds on the previous one, which is the project's preferred incremental-development model.

  • First, do enough so that llvm-dwarfdump can correctly handle an object file with a DW_TAG_immutable_type in it; this generally means tweaking lib/DebugInfo/DWARF a little bit, as I see you have done. The test can be an assembler file with the raw bytes listed; there are many examples of this.
  • Next, make it so textual IR can describe an immutable type, which can be emitted into an object file. The IR part might involve round-tripping through textual and bitcode formats to show that all works. For the object-file part you use llvm-dwarfdump, because your previous work verified that llvm-dwarfdump handles the tag correctly.
  • Next, if you need to fiddle with DIBuilder to get the IR API to handle immutable types, you do that, probably with a unittest (not a Clang test, because LLVM has other frontend clients and LLVM testing should not depend on any of them).
  • Finally, if this is a C/C++ language feature, you plumb it into Clang to emit the tag when appropriate, with a test that uses -emit-llvm and verify that the IR comes out right. I'm not aware that "immutable" is a C/C++ feature, but I find it hard to keep up with the C++ standard.

If the whole thing is trivial enough--which it might be, because this is a type qualifier like many others--you can do multiple phases in one patch, but we still want to see that each step has the proper testing.

Typically I've done "support" for a tag or other DWARF feature in several phases, each usually (not necessarily) with its own patch. Each phase builds on the previous one, which is the project's preferred incremental-development model.

  • First, do enough so that llvm-dwarfdump can correctly handle an object file with a DW_TAG_immutable_type in it; this generally means tweaking lib/DebugInfo/DWARF a little bit, as I see you have done. The test can be an assembler file with the raw bytes listed; there are many examples of this.
  • Next, make it so textual IR can describe an immutable type, which can be emitted into an object file. The IR part might involve round-tripping through textual and bitcode formats to show that all works. For the object-file part you use llvm-dwarfdump, because your previous work verified that llvm-dwarfdump handles the tag correctly.
  • Next, if you need to fiddle with DIBuilder to get the IR API to handle immutable types, you do that, probably with a unittest (not a Clang test, because LLVM has other frontend clients and LLVM testing should not depend on any of them).
  • Finally, if this is a C/C++ language feature, you plumb it into Clang to emit the tag when appropriate, with a test that uses -emit-llvm and verify that the IR comes out right. I'm not aware that "immutable" is a C/C++ feature, but I find it hard to keep up with the C++ standard.

If the whole thing is trivial enough--which it might be, because this is a type qualifier like many others--you can do multiple phases in one patch, but we still want to see that each step has the proper testing.

I did separate patches since they are independent of each other. As a new LLVM contributor, there are some things I miss, and I appreciate this input!

One of the rationales to do this was: since I've seen separate people working in LLVM core and LLDB I decided to separate documentation, and those pieces apart. I thought this could be beneficial for separate reviewers to analyze the change "in the lldb part", "in the llvm core part" and not as a whole. Plus, I noticed that different people get pinged depending on the patch and I assume it has something to do with the patch prefix.

Also, take the example of https://reviews.llvm.org/D113634 , which may be split into two patches for further discussion. Anyway, I tend to agree with you, I can join everything together in D113634.

Also, take the example of https://reviews.llvm.org/D113634 , which may be split into two patches for further discussion. Anyway, I tend to agree with you, I can join everything together in D113634.

Usually (not always, but usually) related patches are split by subproject. So, lldb changes go in one patch, llvm in another patch, clang in another patch. Exceptions occur when patches are very closely related or actually interdependent. I don't think adding immutable_type falls into that category. So, please keep the lldb and llvm patches separate.

ljmf00 edited the summary of this revision. (Show Details)Dec 25 2021, 2:46 PM
ljmf00 updated this revision to Diff 396687.Dec 30 2021, 9:39 AM

Added tests and squashed documentation as requested.

Code looks quite reasonable; the test needs a little cleanup.

llvm/test/DebugInfo/dwarfdump-immutable.ll
4

I believe you can remove the REQUIRES, explicit triple on the llc command, and the target triple IR clause, to allow this to work generically; there doesn't really seem to be anything X86-specific here. You'll want to use %llc_dwarf instead of llc as the command, which will insert an appropriate triple for hosts that don't implicitly use DWARF. See other examples of this in the same test directory.

10

This should be just CHECK: not CHECK-LABEL:.

ljmf00 updated this revision to Diff 397445.Jan 4 2022, 7:02 PM
ljmf00 added inline comments.
llvm/test/DebugInfo/dwarfdump-immutable.ll
4

Done

10

Done

This revision is now accepted and ready to land.Jan 5 2022, 10:44 AM
This revision was landed with ongoing or failed builds.Jan 5 2022, 11:19 AM
This revision was automatically updated to reflect the committed changes.