This is an archive of the discontinued LLVM Phabricator instance.

Switch external cvtres.exe for llvm's own resource library
ClosedPublic

Authored by ecbeckmann on Jun 15 2017, 6:47 PM.

Details

Summary

.In this patch, I flip the switch in DriverUtils from using the externalcvtres.exe tool to using the Windows Resource library in llvm.I also fixed a bug where .rsrc sections were marked as discardable
memory and therefore were placed in the wrong order in the final PE.

Furthermore, I modified WindowsResource to write the coff directly to a
memory buffer instead of to file, also had it use the machine types
already declared in COFF.h instead creating my own enum.

Changed WindowsResourceCOFFWriter to write to a memory buffer instead of file. Also got rid of my own Machine enum and use the one defined in COFF.h

Event Timeline

ecbeckmann created this revision.Jun 15 2017, 6:47 PM
ruiu edited edge metadata.Jun 15 2017, 7:55 PM

As a convention, please write a short summary (ideally less than 80 chars) on the first line of a commit message, so that they are not wrapped around
... like this.

lld/COFF/DriverUtils.cpp
599–604

In LLD we avoid auto unless a concrete type is obvious, so replace auto with a concrete type.

603

Error messages should start with a lowercase letter.

605

And shouldn't end with a full stop. By following that rule, you can concatenate multiple error messages like this.

error while reading file: failed to parse .res file: unexpected EOF
607–612

In LLD we usually use much shorter name. This would be just MB.

llvm/tools/llvm-cvtres/llvm-cvtres.cpp
209

Stray ;.

ecbeckmann added a comment.EditedJun 16 2017, 10:31 AM
In D34265#781998, @ruiu wrote:

As a convention, please write a short summary (ideally less than 80 chars) on the first line of a commit message, so that they are not wrapped around
... like this.

Yes sorry about this. In the commit message just the first sentence is the title and the rest is the summary below. I think arc diff put it all together though.

ruiu added a comment.Jun 16 2017, 10:42 AM

The first line of a commit message should be shorter than that is now. If you look at other commits, you'll find that their first lines are usually less than 80 or 100 characters.

ecbeckmann updated this revision to Diff 102847.EditedJun 16 2017, 11:04 AM
ecbeckmann marked 5 inline comments as done.

Cleaned up style-related errors. Also switched more tests to be cross-platform instead of windows only.

In D34265#782500, @ruiu wrote:

The first line of a commit message should be shorter than that is now. If you look at other commits, you'll find that their first lines are usually less than 80 or 100 characters.

I meant the title should be "Switch external cvtres.exe for llvm's own resource library" which is less than 80 characters.

ruiu accepted this revision.Jun 16 2017, 1:59 PM

LGTM

This revision is now accepted and ready to land.Jun 16 2017, 1:59 PM
This revision was automatically updated to reflect the committed changes.
zturner added inline comments.Jun 16 2017, 3:39 PM
llvm/trunk/include/llvm/Object/WindowsResource.h
184 ↗(On Diff #102874)

Either make this a reference or a pointer, but not a reference to a unique_ptr. Probably a reference is fine since it doesn't make sense to write something without a valid output buffer.

llvm/trunk/lib/Object/WindowsResource.cpp
310–312 ↗(On Diff #102874)

Same thing here.

356 ↗(On Diff #102874)

This is a bit strange. First we initialize the member variable OutputBuffer with the constructor argument (in the initializer list), and then we overwrite.... one of the two items with a new memory buffer? I'm not sure which one takes precedence here, but I think it's probably the constructor arg which means you're treating the constructor arg as an in/out parameter, which is pretty non-standard.

In any case, can you re-think this logic to make it more sensible? Do you intend for this class to take ownership of the input parameter? if so, then pass the unique_ptr by value, not by reference. If not, then pass the MemoryBuffer by reference, and we don't need the unique_ptr at all.

421 ↗(On Diff #102874)

The previous code didn't need a const_cast. Why does the new code?

zturner edited edge metadata.Jun 16 2017, 3:41 PM

Also, sorry for being slow on reviewing this, but feel free to make the suggestions in a follow up patch.

ecbeckmann added inline comments.Jun 16 2017, 4:02 PM
llvm/trunk/lib/Object/WindowsResource.cpp
356 ↗(On Diff #102874)

The idea is that we get an uninitialized reference to a MemBuf from the user, and then initialize it with the final COFF. It must be uninitialized because in order to initialize a MemBuf you must know the size you have to allocate, and this should be calculated only within the WindowsResourceCOFFWriter class. For all intents and purposes this parameter is an output, and is not used as an input in any way.

Therefore passing by reference won't work because we are the ones to initialize the buffer, it can't be done outside. We also cannot pass unique_ptr by value because this parameter needs to be an output accessible to the user, we aren't using it as an input in any way.

Probably the best solution then is to take a MemoryBuffer pointer as input?

421 ↗(On Diff #102874)

Because you can write to a FileOutputBuffer, however MemoryBuffer was implemented to be read-only for some reason. We must use the MemoryBuffer class because this is how Driver(Util) in LLD is currently implemented. There is precedent in other places of the codebase for using const_cast to write to MemoryBuffer, such as in NativeProcessNetBSD and ScratchBuffer.

zturner added inline comments.Jun 16 2017, 4:08 PM
llvm/trunk/lib/Object/WindowsResource.cpp
356 ↗(On Diff #102874)

In that case instead of returning an Error, how about returning an Expected<unique_ptr<MemoryBuffer>> from writeWindowsCOFFResource?

ecbeckmann added inline comments.Jun 16 2017, 4:25 PM
llvm/trunk/lib/Object/WindowsResource.cpp
356 ↗(On Diff #102874)

That seems like the best option

ecbeckmann retitled this revision from Switch external cvtres.exe for llvm's own resource library. In this patch, I flip the switch in DriverUtils from using the external cvtres.exe tool to using the Windows Resource library in llvm. I also fixed a bug where .rsrc sections were... to Switch external cvtres.exe for llvm's own resource library.Aug 7 2017, 6:19 PM
ecbeckmann edited the summary of this revision. (Show Details)