This is an archive of the discontinued LLVM Phabricator instance.

Dump swift5 reflection section data into dsym bundle generated binary with dsymutil
ClosedPublic

Authored by rastogishubham on Dec 2 2021, 4:22 PM.

Details

Summary

This change allows dsymutil to dump swift5 reflection section data into the binary that is created by dsymutil in the dsym bundle.

We only want to emit these data of these sections if the input binary does not have those sections present in them. Therefore, assuming that the input binary doesn't have the section data, dsymutil is supposed to pick the data up from the object files. If the Input binary has the data, dsymutil works the same as it has before this change.

Diff Detail

Event Timeline

rastogishubham created this revision.Dec 2 2021, 4:22 PM
rastogishubham requested review of this revision.Dec 2 2021, 4:22 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: sstefan1. · View Herald Transcript

Thanks, this is looking pretty good!

llvm/include/llvm/MC/MCObjectFileInfo.h
431

I know the other comments in this file also don't use this format, but FYI, you can doxygenify this as:

/// Swift5 Reflection Data Sections.
/// \{
MCSection *getSwift5FieldmdSection() { return Swift5FieldmdSection; }
  MCSection *getSwift5AssoctySection() { return Swift5AssoctySection; }
  MCSection *getSwift5BuiltinSection() { return Swift5BuiltinSection; }
  ...
/// \}
llvm/include/llvm/Object/ObjectFile.h
37

I think this fits better into the BinaryFormat directory. I would even consider adding a swift.h header there. After all that's also where the DWARF sections are defined (in Dwarf.def/Dwarf.h).

44

maybe "Unknown = 0" ?

llvm/lib/DWARFLinker/DWARFStreamer.cpp
313

Maybe just have a MOFI->getSwiftReflectionSection(ReflSectionType) function?

330

it's better to explicitly list all cases instead of having a default: label. This way we get a warning when someone adds a new case to the enum that isn't covered by the switch.

llvm/lib/MC/MCObjectFileInfo.cpp
302

Only dsymutil puts these into the __DWARF section. We need to at least comment this, but if we can make this configurable, that would be even better.

llvm/test/tools/dsymutil/Inputs/main.yaml
5117

It's great to have this in YAML form. Do you think it might be possible to strip out parts of the executable that aren't strictly necessary for testing the dsymutil functionality? Maybe by running delta on the yaml file and test that the test still passes? I understand that some of the hardcoded offsets in the yaml might might make this hard.

llvm/test/tools/dsymutil/reflection-dump.test
1 ↗(On Diff #391487)

RUN: rm -rf %t.dir && mkdir -p %t.dir/tmp

9 ↗(On Diff #391487)

Are there any error cases that we'd want to test?

10 ↗(On Diff #391487)

Nice testcase!

llvm/tools/dsymutil/DwarfLinkerForBinary.cpp
299

Does binaryHasSwiftReflectionSections() sound better? (not sure)

303

// If the input binary has swift5 reflection sections, there is no need to copy them to the .dSYM. Only copy them for binaries where the linker omitted the reflection metadata.

310

Nit: LLVM style always wants a . at the end of each sentence.

474

maybe also factor this into a copySwiftReflectionMetadata() function?

rastogishubham marked 13 inline comments as done.Dec 8 2021, 9:48 AM
rastogishubham added inline comments.
llvm/include/llvm/MC/MCObjectFileInfo.h
431

Good to know, would it be weird to doxygen just the functions we added though?

llvm/lib/DWARFLinker/DWARFStreamer.cpp
313

I think that just moves the switch statement into a different function, and there is no overt benefit to doing that.

llvm/lib/MC/MCObjectFileInfo.cpp
302

The only thing I can think about doing here is making a function that sets the swift5 sections and takes a const char* for the section name, does that work?

llvm/test/tools/dsymutil/reflection-dump.test
1 ↗(On Diff #391487)

Thank you

9 ↗(On Diff #391487)

The only error I saw was if you messed up the yaml files, then the otool output would come out wrong, not sure what else to check for. Any suggestions? I guess one thing I can test is that when the input binary does have the reflection metadata sections, then we should not emit them in the output. Not really an error test case though.

10 ↗(On Diff #391487)

Thank you!

llvm/tools/dsymutil/DwarfLinkerForBinary.cpp
299

I have always considered myself bad with names, so I am gonna go with yours :)

310

Thanks!

rastogishubham marked 8 inline comments as done.

Added the changes as mentioned by Adrian, based on my comments in response to him

rastogishubham updated this revision to Diff 393565.EditedDec 10 2021, 12:22 PM
rastogishubham marked an inline comment as done.

Reduced the testcase yaml files, made the Swift5ReflectionSection creation confirgurable, also moved the copying of the reflection section metadata to its own function called copySwiftReflectionMetadata

Thanks for reducing the test! This looks much more palatable.

llvm/include/llvm/BinaryFormat/Swift.h
15

If it's already in the namespace we don't need to repeat Swift in the name. I would go with:

enum Swift5ReflectionSectionKind {
  Unknown = 0,
  Fieldmd,
  ...
};

or 

enum ReflectionSectionKind {
  Unknown = 0,
  Swift5Fieldmd,
  ...
};
22

We might want to split this out into a .def file. See Dwarf.def for an example:

Swift.def:

#ifdef HANDLE_SWIFT_SECTION
#undef HANDLE_SWIFT_SECTION
#endif
// Enum Kind, Mach-O name, ELF name, COFF name.
HANDLE_SWIFT_SECTION(Swift5Fieldmd, "__swift5fieldmd", ".swift5fieldmd", ???)

Swift.h

enum ReflectionSectionKind {
  Unknown = 0,
#define HANDLE_SWIFT_SECTION(KIND, MACHO, ELF, COFF) #KIND,
#include "llvm/BinaryFormat/Swift/def"
};
llvm/include/llvm/DWARFLinker/DWARFStreamer.h
90

I would drop the "5" here.

llvm/include/llvm/MC/MCObjectFileInfo.h
232

Here the Swift"5" makes sense.

431

I would do a second NFC patch to just doxygenify the entire class.

447

this should be a StringRef

llvm/lib/MC/MCObjectFileInfo.cpp
302

I would just write:

// The architecture of dsymutil makes it very difficult to copy the Swift reflection metadata sections into the __TEXT segment, so dsymutil creates these sections in the __DWARF segment instead.
310

If you create a Swift.def file, this can also be just a single #include with some clever macro substitutions. Check out the dwarf names are stringified for an example.

1022

The DWARF segment needs to be hardcoded in dsymutil or DwarfLinker. The MC layer should not know about this. So we need to thread the flag all the way to DwarfLinker.

llvm/lib/Object/MachOObjectFile.cpp
4725

If you create a Swift.def file, this can also be just a single #include with some clever macro substitutions. Check out the dwarf names are stringified for an example.

rastogishubham marked 10 inline comments as done.Dec 14 2021, 1:51 AM

Added a new file called Swift.def, it contains macros that define the various swift reflection section names and contents so we can use them throughout the code, and code changes that come with using the def file. Also added a change to have dwarflinker pass in the segment "__DWARF" to the streamer and the MC to create the reflection sections in the dwarf segment

Fixed comments

Thanks for the update! We're down to cosmetic changes.

llvm/include/llvm/BinaryFormat/Swift.def
27

missing newline

27

This is nice.

llvm/include/llvm/DWARFLinker/DWARFStreamer.h
91

s/type/kind/

llvm/include/llvm/MC/TargetRegistry.h
394 ↗(On Diff #394191)

Instead of passing a StringRef as a pointer, we can just pass a StringRef value, like this:

StringRef Swift5ReflectionSegmentName = {}

and then check for .empty() instead for nullptr.

llvm/lib/DWARFLinker/DWARFStreamer.cpp
313

What's potentially more elegant is to have a std::array of reflection sections in MOFI instead of 6 separate member variables. If you change Unknown to -1 and let Fieldmd be 0 you could access it as Swift5ReflectionSection[Swift5ReflectionSectionType::Fieldmd]. Then we could replace the 6 accessor functions with one getSwiftReflectionSection(Swift5ReflectionSectionType)

llvm/lib/MC/MCObjectFileInfo.cpp
52

just StringRef

311

nice!

311

is this redundandant?

1000

StringRef

llvm/test/tools/dsymutil/reflection-dump.test
42 ↗(On Diff #394191)

newline

rastogishubham marked 10 inline comments as done.Dec 14 2021, 12:44 PM
rastogishubham added inline comments.
llvm/lib/MC/MCObjectFileInfo.cpp
311

Yes it is definitely redundant, didn't think of the scope

rastogishubham marked an inline comment as done.

Removed StringRef pointer and replaced it with a default empty StringRef argument. Changed Unkown to -1 and removed distinct swift5 sections in MOFI to one std::array. Changed names from Swift5ReflectionSectionType to Swift5ReflectionSectionKind.

aprantl added inline comments.Dec 14 2021, 1:35 PM
llvm/include/llvm/BinaryFormat/Swift.h
19

You could make this Unknown, Last = Unknown see comment below.

llvm/include/llvm/MC/MCObjectFileInfo.h
435

Nice.

aprantl added inline comments.Dec 14 2021, 1:37 PM
llvm/include/llvm/MC/MCObjectFileInfo.h
234
  1. Instead of hardcoding 6, does std::array<MCSection *, Swift5ReflectionSectionKind::Last> work?
  1. = {}; is shorter than listing all the nullptrs.
  1. I would call this Swift5ReflectionSections
JDevlieghere requested changes to this revision.Dec 15 2021, 10:28 AM
JDevlieghere added inline comments.
llvm/include/llvm/MC/MCObjectFileInfo.h
233

Is there a way we can keep this in sync automatically with the number of sections in Swift.def?

239

Should this be part of MCContext?

447

Similar as above, this seems really out of place.

llvm/tools/dsymutil/DwarfLinkerForBinary.cpp
307

Do we really need to create a new binary holder? Why can't we use the one used by the DWARFLinker?

This revision now requires changes to proceed.Dec 15 2021, 10:28 AM
rastogishubham marked 7 inline comments as done.Dec 20 2021, 2:07 PM
rastogishubham added inline comments.
llvm/include/llvm/MC/MCObjectFileInfo.h
239

Sure!

447

I don't think this makes sense, I do not think the swift5reflectionsegmentname should be part of the Triple, so it has to be it's own argument.

llvm/tools/dsymutil/DwarfLinkerForBinary.cpp
307

Did not realize there was one, but I am not sure why

bool generateDsymCompanion(llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS,
                           const DebugMap &DM, SymbolMapTranslator &Translator,
                           MCStreamer &MS, raw_fd_ostream &OutFile) {

in MachoUtils.cpp creates it's own BinaryHolder?

rastogishubham marked 3 inline comments as done.

Put StringRef Swift5ReflectionSegmentName in MCContext, made the SwiftReflectionSections Array the size of enum::Last instead of harcoding.

aprantl added inline comments.Dec 20 2021, 2:42 PM
llvm/include/llvm/MC/MCObjectFileInfo.h
447

Similar as above, this seems really out of place.

@JDevlieghere can you clarify what you mean? We need to make the reflection segment name configurable. What is it that you are suggesting?

rastogishubham added inline comments.Jan 6 2022, 8:40 PM
llvm/include/llvm/MC/MCObjectFileInfo.h
447

@JDevlieghere Hi Jonas, can you please clarify what you mean? As Adrian said we need to make the segment name configurable, and I don't think it is valid to make that part of the Triple.

Thanks!

JDevlieghere added inline comments.Jan 11 2022, 9:13 AM
llvm/include/llvm/MC/MCObjectFileInfo.h
447

I meant that I think it's weird that generic MC interfaces need to know about Swift section names. I understand the need to pass the information, I just don't think it should be part of the interface like this. If I follow the code correctly, the Swift 5 reflection segment name is stored in the MCContext. Instead of passing this the Swift5ReflectionSegmentName in here, can it query the *Ctx for the name? That would address my concern.

rastogishubham marked an inline comment as done.Jan 11 2022, 3:40 PM
rastogishubham added inline comments.
llvm/include/llvm/MC/MCObjectFileInfo.h
447

Oh I don't know why I didn't realize that! Yes the ctx is part of the same MCObjectFileInfo object, so instead of passing it as an argument, it should just be something we should query from within the method itself!

Thank you Jonas, really appreciate your help here!

rastogishubham marked an inline comment as done.

Removed the argument Swift5ReflectionSectionName from initMachOMCObjectFileInfo function and get the name from the Ctx object present within the MCObjectFileInfo object instead

JDevlieghere added inline comments.Jan 13 2022, 10:35 AM
llvm/tools/dsymutil/DwarfLinkerForBinary.cpp
299

Instead of having an in-our argument, could this function return an Expected<bool> instead? I also find NoError confusing because you end up checking a double negation, i.e. !NoError.

300

Why are the LinkOptions not const?

348–351

You' assuming that these pointers are non-null so why not pass DebugMapObject and Streamer by references? If that's not an option, I'd just pass the underlying pointers instead. I can't think of any benefit of passing the unique_ptr by reference instead.

357–358

A common style in LLVM is to do if(Foo* foo = ...) {.

366
474–476

No braces around single-line if's (https://llvm.org/docs/CodingStandards.html#id59)

rastogishubham marked 6 inline comments as done.

Changed the logic for binaryHasSwiftReflectionSections to remove the NoError bool flag, made LinkOpts const, passed DebugMapObject and DwarfStreamer Objects as pointers to copySwiftReflectionMetadata, removed auto where it isn't obvious, removed braces from single line if, used llvm style if statement for dyn_cast

llvm/tools/dsymutil/DwarfLinkerForBinary.cpp
299

I realized that NoError is not needed, the function just needs to return if there are reflectionsections in the input binary. So a simple return bool value should be enough, the reason is because creating the ObjectEntry and Object are handled in MachoUtils.cpp anyway.

aprantl added inline comments.Jan 18 2022, 6:06 PM
llvm/tools/dsymutil/DwarfLinkerForBinary.cpp
305

I don't think the Map.getTriple().isOSDarwin() && check is necessary.

312

It would be good to comment here:

// Any errors will be diagnosed later in the main loop, ignore them here.
``
rastogishubham marked 2 inline comments as done.

Added the comments as Adrian suggested, and removed the check Adrian suggested as well

JDevlieghere accepted this revision.Jan 19 2022, 3:39 PM

LGTM modulo the early return.

llvm/tools/dsymutil/DwarfLinkerForBinary.cpp
347–349

Seems like an early return here could save you a level of indentation.

This revision is now accepted and ready to land.Jan 19 2022, 3:39 PM
rastogishubham marked an inline comment as done.

Added early return and change else { if .. to and else if

rastogishubham updated this revision to Diff 402601.EditedJan 24 2022, 11:14 AM

In DwarfLinkerForBinary.cpp Line 475, we need to make sure that copySwiftReflectionMetadata is not called when the -no-output option is passed, this is because the DwarfStreamer object is not created, and copySwiftReflectionMetadata calls through to a member method of the DwarfStreamer Object. I also moved reflection-metadata.test to the X86 folder because some non X86 tests failed due to target triple issues

aprantl accepted this revision.Jan 26 2022, 10:53 AM
aprantl added inline comments.
llvm/tools/dsymutil/DwarfLinkerForBinary.cpp
473

lower case for "Reflection Sections" ?

rastogishubham marked an inline comment as done.Jan 26 2022, 1:43 PM
This revision was landed with ongoing or failed builds.Jan 26 2022, 2:14 PM
This revision was automatically updated to reflect the committed changes.