[llvm-dev] [llvm-rc] absolute.test failing

[Nico Weber via llvm-dev]

On Thu, Jan 10, 2019 at 3:01 AM Martin Storsjö <martin at martin.st> wrote:

> On Wed, 9 Jan 2019, David Greene wrote:
>
> > I've come across a curious and pernicious problem in llvm-rc.
> > absolute.test checks that llvm-rc can accept a filename that is an
> > absolute path.  And it works just fine.  Until you run it with a file
> > that starts with "/c."
>
> Hmm, that's rather unfortunate indeed.
>
> FWIW, this test doesn't test specifically whether llvm-rc can accept an
> absolute filename as command line argument - all the llvm-rc tests run
> llvm-rc with absolute filenames as arguments. This test checks whether
> llvm-rc can handle an absolute filename reference within a rc file.
>
> I presume you run into the same issue on all other tests in
> test/tools/llvm-rc as well?
>
> > These will fail:
> >
> > llvm-rc /crawl/through/some/path/to/my.rc
> > llvm-rc /c/some/path/to/my.rc
> >
> > The option parser ends up interpreting "/" as an option prefix and then
> > the parser matches it to this in tools/llvm-rc/Opts.td:
> >
> > def CODEPAGE : JoinedOrSeparate<[ "/", "-" ], "C">,
> >               HelpText<"Set the codepage used for input strings.">;
> >
> > The test then fails with:
> >  Exactly one input file should be provided.
> >
> > The same problem happens with files that begin with "/r"
> > (/read/the/path/to/my.rc) "/sl" (/slink/along/the/path/to/my.rc) or any
> > other path that happens to begin with the same text as an option in
> > Opts.td.
> >
> > This triggered on one of our builders that just happens to build to a
> > path that begins with "/c."  Presumably none of the existing Buildbots
> > build to paths that cause problems.
> >
> > It's easy enough to construct a test for this, but I'm not sure how/if
> > llvm-rc should be fixed.  I don't know why it accepts both "/" and "-"
> > as option prefixes.  As this mostly seems related to Windows (resource
> > files), should tests be UNSUPPORTED on every other platform?  Or is
> > llvm-rc intended to be a cross-platform way to create resource files?
>
> It's definitely intended as a cross-platform tool for generating windows
> resource files, to allow for cross compilation etc.
>
> > If the latter, then it seems like options ought to use the "/" prefix on
> > Windows and "-" everywhere else so as not to conflict with path
> > specifiers.
>
> Well, build scripts that call llvm-rc might be using either (more or less
> agnostic of what platform it runs on). I personally prefer always using
> "-" everywhere though (which also is supported on windows, and also
> supported by the original microsoft tools, even if their help listings
> only display the form with a "/").
>
> FWIW, lld-link also implements the same form of options using both
> prefixes, but there's less risk of unintended matches as most option names
> are full words, not single-char abbreviations.
>
> One way of disambiguating between option and pathname for the sake of the
> tests, would be to add '--' before the path arguments, which seems to be
> handled by the LLVM options parser at least. Does that sound sensible to
> you (and others CC:d)?
>

That sounds like the thing to do. It's also how we handle the same issue in
clang-cl, where /Users/foo/file.c is interpreted as the /U (undefine macro)
flag with"sers/foo/file.c" as argument instead of the intended macOS-style
path.

clang-cl also has a dedicated warning for this (
http://reviews.llvm.org/rL293305), might be useful for some of llvm-rc's
flags that are prone to this problem as well (/c, /r, maybe /sl)


>
> // Martin
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20190110/703f9e6f/attachment.html>