[llvm-rc] absolute.test failing

Project Infrastructure LLVM Dev List Archives
1

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."

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?
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.

                             -David

2

I believe Windows uses slashes instead of hyphens for command line args.

Liam

3

Ha, that was unhelpful. I read your email just shy of the point where you mention Windows.

Sorry for the noise.

4

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)?

// Martin

5

It can use both. The is no issue with the slashes however since Windows tools use backwards slashes for paths.

It's unfortunate that this conflicts with the expected behavior on *nix.
IMHO, ideally it should be opt-in, not opt-out (i.e. the users shouldn't need to use workarounds in default case).

Danila

6

Just use — before the file name. This comes up in other places with other tools as well. For example, if a file starts with /U clang-cl will interpret it as an option. This turns out to be extremely common on Mac since home directories are under/Users

7

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)

8

Martin Storsjö <martin@martin.st> writes:

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?

Actually, I don't. Now I wonder why...

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)?

I tried that and it didn't work. Same error.

$ ./bin/llvm-rc -- /choose/me
Exactly one input file should be provided.

                        -David

9

Oh, then apparently something extra has to be done to get this behaviour out of the -- parameter; I only tested that it wasn't rejected. Will have a look soon.

// Martin

10

      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.

This should be implemented now and taken into use in the tests, in rG58bb0e47dcd4. Does this help with your issue?

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

That'd probably be a nice extra to implement on top of it, although the case where it struck for David wasn't when manually using the tool, but within the testsuite. Nevertheless it probably would be useful.

// Martin