[llvm-rc] Serialize HTML resources to .res files (serialization, pt 1).
ClosedPublic

Authored by mnbvmar on Aug 29 2017, 5:58 PM.

Details

Summary

This allows to process HTML resources defined in .rc scripts and output them to resulting .res files. Additionally, some infrastructure allowing to output these files is created.

This is the first resource type we can operate on.

Diff Detail

Repository
rL LLVM
mnbvmar updated this revision to Diff 113184.Aug 29 2017, 6:08 PM

One file was missing from the patch.

ecbeckmann added inline comments.Aug 29 2017, 7:12 PM
llvm/tools/llvm-rc/ResourceScriptStmt.cpp
148 ↗(On Diff #113184)

It would be good if we could reuse the .res file structures defined in WindowsResource.h, to ensure the sizes of each field are kept in sync and also have a single unified place in the code for defining the .res format. So instead of writing each field individually, you could define a local struct WinResHeaderPrefix and then call Writer.write<struct WinResHeaderPrefix>(the struct)

mnbvmar updated this revision to Diff 113295.Aug 30 2017, 11:55 AM

Changed the code according to Eric's suggestion; some bugfixes.

mnbvmar marked an inline comment as done.Aug 30 2017, 11:55 AM
ecbeckmann added inline comments.Sep 1 2017, 11:36 AM
llvm/tools/llvm-rc/ResourceScriptStmt.cpp
117 ↗(On Diff #113295)

Instead of an overload you should rename this to writeIntOrString for clarity.

mnbvmar updated this revision to Diff 115081.Sep 13 2017, 11:42 AM
mnbvmar retitled this revision from [llvm-rc] Output .res files for HTML resource. to [llvm-rc] Serialize HTML resources to .res files (serialization, pt 1)..

Refactored the code to simplify the patches following this one.

Previously, resource classes had ability to output themselves, using and possibly modifying the current context. Now, Visitor class is introduced which holds the current context and processes resources one by one and serializes them.

This still holds the ability to output HTML resources.

mnbvmar updated this revision to Diff 115144.Sep 13 2017, 5:48 PM

This fixes @ecbeckmann's comment https://reviews.llvm.org/D37824#inline-329768 (set type IDs and memory flags as constants). It's probably better to fix this in the first patch this appears in.

rnk added inline comments.Sep 15 2017, 3:45 PM
llvm/test/tools/llvm-rc/tag-html.test
2–4

nit: personally I like RUN: rm -rf %t && mkdir %t && cd %t as the standard one-line idiom for "get this test a clean working directory"

7

Do we have any tools that can dump resource files? These kind of "golden file" tests are pretty fragile and aren't easy to debug when they fail.

llvm/tools/llvm-rc/ResourceSerializator.h
1 ↗(On Diff #115144)

Even if "Serializator" is a word, I'd recommend something more common, like ResourceFileWriter or something.

56 ↗(On Diff #115144)

Maybe call this writeBytes, and take ArrayRef<uint8_t>?

58 ↗(On Diff #115144)

I'm concerned that an object could easily contain endian-swapped fields, but I can't think of a clean way to defend against it, so let's go with this.

mnbvmar added inline comments.Sep 19 2017, 10:27 AM
llvm/test/tools/llvm-rc/tag-html.test
7

We could pipe the output of llvm-cvtres to llvm-readobj. However, cvtres needs to reorder the resources, and readobj can only extract header information.

llvm/tools/llvm-rc/ResourceSerializator.h
58 ↗(On Diff #115144)

The only thing I can think of is some std::disable_if detecting basic integral types larger than a byte. This is nowhere close to perfect, though.

zturner added inline comments.Sep 20 2017, 9:45 AM
llvm/tools/llvm-rc/ResourceSerializator.h
56 ↗(On Diff #115144)

It seems like all of these functions are reproducing logic that already exists in BinaryStreamWriter. Is there some reason we can't use that here?

The nice thing about them is they prevent endianness errors up front, because you don't have to remember to call writeObject() with some endian-aware type, because as soon as you set the endianness on the stream, all integers get written appropriately.

It also contains functions for writing c strings, arrays, arbitrary objects, and seeking, so it seems like a good fit here and reduces some boilerplate?

llvm/tools/llvm-rc/llvm-rc.cpp
156

Can this be a unique_ptr?

mnbvmar added inline comments.Sep 20 2017, 10:37 AM
llvm/tools/llvm-rc/ResourceSerializator.h
56 ↗(On Diff #115144)

BinaryStreamWriter expects to know the size of the underlying buffer beforehand, so I can't really use it here.

However, is it a good idea to teach this to write little-endian integers only (and convert native-endian ints to ulittle{}_t when necessary)?

zturner added inline comments.Sep 20 2017, 10:45 AM
llvm/tools/llvm-rc/ResourceSerializator.h
56 ↗(On Diff #115144)

Yes, just call it writeInteger and do the endian conversion inside the function. Then call the function writeInteger<uint64_t>(12);

mnbvmar updated this revision to Diff 116123.Sep 20 2017, 5:33 PM

Style fixes suggested in the comments. Integrated llvm-readobj into test invocation.

(I changed file and class names to ResourceFileWriter. However, I leave the revision as-is not to make confusion.)

mnbvmar marked 4 inline comments as done.Sep 20 2017, 5:34 PM
rnk accepted this revision.Sep 21 2017, 2:11 PM

lgtm, thanks!

This revision is now accepted and ready to land.Sep 21 2017, 2:11 PM
mnbvmar closed this revision.Sep 29 2017, 10:51 AM

Seems this revision didn't close automatically. Fixing that.