This is an archive of the discontinued LLVM Phabricator instance.

ObjectFile: introduce a COFF object file plugin
ClosedPublic

Authored by compnerd on May 5 2023, 12:54 PM.

Details

Summary

Windows uses COFF as an object file format and PE/COFF as an executable
file format. They are subtly different and certain elements of a COFF
file may not be present in an executable. Introduce a new plugin to add
support for the COFF object file format which is required to support
loading of modules built with -gmodules. This is motivated by Swift
which serialises debugging information into a PCM which is a COFF object
file.

Diff Detail

Event Timeline

compnerd created this revision.May 5 2023, 12:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 5 2023, 12:54 PM
compnerd requested review of this revision.May 5 2023, 12:54 PM
compnerd added a reviewer: rnk.

Could you build this into a yaml2obj obj2yaml plugin, so we could test this?
Alternatively, you could check in a minimal binary.

Is there a way to compile something with clang and then load the object file into lldb-test so we could add a shell test for it?

We're definitely going to want a test for this, either a shell test or a unit test.

lldb/source/Plugins/ObjectFile/COFF/ObjectFileCOFF.cpp
104–105

Reading the implementation of the constructor, it looks like the constructor can fail to initialize correctly (specifically m_object may not be correctly populated). What are callers supposed to do in the way of validation here? Maybe there is further validation we can do in this function so that the constructor is only invoked if we're absolutely sure it will work?

compnerd added inline comments.May 5 2023, 2:05 PM
lldb/source/Plugins/ObjectFile/COFF/ObjectFileCOFF.cpp
104–105

There isn't much you can do IMO. The new can fail just as well - at which point, what do we do? The constructor should only really fail if the return type from libLLVMObject has suddenly changed into an invalid type. That cast really cannot fail in a way that we can recover from.

bulbazord added inline comments.May 5 2023, 2:15 PM
lldb/source/Plugins/ObjectFile/COFF/ObjectFileCOFF.cpp
104–105

createBinary and the subsequent cast may not fail in a recoverable fashion but doing it in the constructor means that whatever is trying to create an object gets back an ObjectFileCOFF object even if it wasn't initialized correctly. If you did that work in CreateInstance, you could return nullptr if createBinary and that cast failed.

compnerd added inline comments.May 5 2023, 2:50 PM
lldb/source/Plugins/ObjectFile/COFF/ObjectFileCOFF.cpp
104–105

Ah, sinking that into this seems plausible, and then the constructor takes the binary? WFM!

compnerd updated this revision to Diff 519984.May 5 2023, 2:58 PM

Address feedback, add test

This revision is now accepted and ready to land.May 5 2023, 3:06 PM
aprantl added inline comments.May 5 2023, 3:33 PM
lldb/source/Plugins/ObjectFile/COFF/ObjectFileCOFF.cpp
204

Can these be correct? They seem too long.

lldb/source/Plugins/ObjectFile/COFF/ObjectFileCOFF.h
17

Can you add a short doxygen comment?

87

We usually don't sign our FIXMEs :-)

tschuett added inline comments.
lldb/source/Plugins/ObjectFile/COFF/ObjectFileCOFF.cpp
27

Could you turn this into a static instead?

compnerd marked 4 inline comments as done.May 6 2023, 9:53 AM
compnerd added inline comments.
lldb/source/Plugins/ObjectFile/COFF/ObjectFileCOFF.cpp
204

They do exceed the COFF limit of 8 characters. However, this is what is actually emitted with a tweak to indicate the long name. This is why I cannot simply use the Section->Name and need to extract the long name through the generic SectionRef.

compnerd updated this revision to Diff 520102.May 6 2023, 9:54 AM
compnerd marked an inline comment as done.

Address feedback

This revision was automatically updated to reflect the committed changes.
lkail added a subscriber: lkail.May 7 2023, 12:52 PM