Page MenuHomePhabricator

[llvm-rc] [4/4] Add a GNU windres-like frontend to llvm-rc
ClosedPublic

Authored by mstorsjo on Apr 19 2021, 5:28 AM.

Details

Summary

This primarily parses a different set of options and invokes the same
resource compiler as llvm-rc normally. Additionally, it can convert
directly to an object file (which is done with the separate cvtres tool,
or by the linker, in MSVC style setups).

(GNU windres also supports other conversions; from coff object file back
to .res, and from .res or object file back to .rc form; that's not yet
implemented.)

The other bigger complication lies in being able to imply or pass the
intended target triple, to let clang find the corresponding mingw sysroot
for finding include files, and for specifying the default output object
machine format.

It can be implied from the tool triple prefix, like
<triple>-[llvm-]windres, picked up from the windres option -F pe-x86-64
(where only BFD style format names are supported; as libbfd in binutils
doesn't support Windows on ARM, there's no such canonical name for the
ARM targets. Therefore, as an LLVM specific extension, this option is
extended to allow passing full triples.)

Diff Detail

Event Timeline

mstorsjo created this revision.Apr 19 2021, 5:28 AM
mstorsjo requested review of this revision.Apr 19 2021, 5:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2021, 5:28 AM
rnk resigned from this revision.Apr 19 2021, 1:50 PM
mstorsjo updated this revision to Diff 338783.Apr 20 2021, 2:38 AM

Fixed the use of temp files so tests pass on windows, added missing dependencies for the new tool.

CCing some more people involved with mingw tools, in case they have comments on some of the details here.

mstorsjo updated this revision to Diff 338978.Apr 20 2021, 1:28 PM

Rebased on top of the updated preceding patch. Adjusted the logic for picking th
e default triple, to preserve the exact spelling of the default triple, if isWindowsGNUEnvironment() is true. Added a preprocessing test under clang.

Herald added a project: Restricted Project. · View Herald TranscriptApr 20 2021, 1:28 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
mstorsjo updated this revision to Diff 339158.Apr 21 2021, 3:01 AM

Rebased, updated with fixes making the full preprocessing test pass on windows, including fixes for the default arch when running on arches that aren't normally supported as windows targets.

aganea added inline comments.Apr 21 2021, 8:09 AM
llvm/tools/llvm-rc/llvm-rc.cpp
259–264

s/str/Str/

300

I would also need this function in D43002 (see unescapeSlashes), do you think we can move it to sys::path or any other "support" lib?

568

There was a latent issue here - unrelated to the moving of code around - I don't know if you want to fix it now or after? That is, s/16/0/.

mstorsjo added inline comments.Apr 21 2021, 8:49 AM
llvm/tools/llvm-rc/llvm-rc.cpp
259–264

Thanks, will fix.

300

This does things differently than your function - this converts \\\" into \" while your version leaves \\" if I read it correctly.

(I'm also considering a different, platform specific unescaping strategy - that'd be closer to how GNU windres behaves, but makes for a more inconsistent tool. Keeping the logic here makes it easier to tweak if needed.)

568

Hmm, as far as I can see, both rc.exe and GNU windres interpret values as hexadecimal - if I run e.g. rc.exe -l 40 test.rc, where 40 is ambiguous, I get language id 64.

mstorsjo updated this revision to Diff 339263.Apr 21 2021, 9:02 AM

Updated the capitalization of one variable

aganea added inline comments.Apr 21 2021, 9:41 AM
llvm/tools/llvm-rc/llvm-rc.cpp
300

I think unescapeSlashes in D43002 was only a quick hack to allow copy-pasting a command-line file name from LIT outputs into the VS debugger options dialog. If the file name contains double quotes I would expect it to be converted as you do, but double quotes cannot happen in file names on Windows. In theory one could use the S_OBJNAME and LF_BUILDINFO records to repro a clang-cl build command on Linux, but that seems improbable? (since the feature is meant to be used on Windows)

568

Ok, we only have stuff like rc.exe /l 0x0409 in our build scripts, the 0x in front isn't consumed, I though it was mandatory.

mstorsjo marked an inline comment as done.Apr 21 2021, 11:25 AM
mstorsjo added inline comments.
llvm/tools/llvm-rc/llvm-rc.cpp
300

Hmm, right. In any case - for that patch, I guess one would have to specify which unescaping one expects to do.

This one here is (currently) meant to be unix shell unescaping, but it could also be unix/windows shell unescaping based on what platform it runs on.

568

Ah, I see. I guess that's just good form to be explicit as user, as it indeed is quite ambiguous otherwise.

mstorsjo marked an inline comment as done.Apr 22 2021, 12:40 PM

Any further comments on this one, or is tolerable in this form? I'd prefer to not move the unescaping to shared code for now (as the exact definition of what it should do is a bit open).

amccarth accepted this revision.Apr 26 2021, 10:49 AM

There's a lot going on here, but I don't see anything wrong. Thanks for the completeness of the tests and the comments, as that helps a lot in understanding what's going on here.

llvm/tools/llvm-rc/llvm-rc.cpp
295

".obj"?

This revision is now accepted and ready to land.Apr 26 2021, 10:49 AM

There's a lot going on here, but I don't see anything wrong. Thanks for the completeness of the tests and the comments, as that helps a lot in understanding what's going on here.

Thanks!

With this in the tree, it'll be easier to reason about further requests to the preprocessing, when both use cases are available at once.

llvm/tools/llvm-rc/llvm-rc.cpp
295

Good point, I'll add that too.

This revision was landed with ongoing or failed builds.Apr 26 2021, 12:06 PM
This revision was automatically updated to reflect the committed changes.

thanks for this patch @mstorsjo ! This is help AOSP LLVM move our windows builds of LLVM away from MinGW to LLVM!
https://android-review.googlesource.com/c/toolchain/llvm_android/+/1867380/

thanks for this patch @mstorsjo ! This is help AOSP LLVM move our windows builds of LLVM away from MinGW to LLVM!
https://android-review.googlesource.com/c/toolchain/llvm_android/+/1867380/

Thanks for letting me know - that’s nice to hear that it benefits you too!

Btw, a little nitpick about naming - MinGW is the SDK/environment that you’re still using, but you moved from GNU binutils windres to llvm windres - it’s just as little/much mingw as before. :P