David Greene via llvm-dev
2019-Jan-09 22:03 UTC
[llvm-dev] [llvm-rc] absolute.test failing
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
Liam Reimers via llvm-dev
2019-Jan-09 23:20 UTC
[llvm-dev] [llvm-rc] absolute.test failing
I believe Windows uses slashes instead of hyphens for command line args. Liam> On Jan 9, 2019, at 2:03 PM, David Greene via llvm-dev <llvm-dev at lists.llvm.org> 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." > > 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 > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Liam Reimers via llvm-dev
2019-Jan-09 23:22 UTC
[llvm-dev] [llvm-rc] absolute.test failing
Ha, that was unhelpful. I read your email just shy of the point where you mention Windows. Sorry for the noise.> On Jan 9, 2019, at 3:20 PM, Liam Reimers <wreimers at apple.com> wrote: > > I believe Windows uses slashes instead of hyphens for command line args. > > Liam > >> On Jan 9, 2019, at 2:03 PM, David Greene via llvm-dev <llvm-dev at lists.llvm.org> 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." >> >> 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 >> _______________________________________________ >> LLVM Developers mailing list >> llvm-dev at lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >
Martin Storsjö via llvm-dev
2019-Jan-10 08:01 UTC
[llvm-dev] [llvm-rc] absolute.test failing
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)? // Martin
Danila Malyutin via llvm-dev
2019-Jan-10 10:21 UTC
[llvm-dev] [llvm-rc] absolute.test failing
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 -----Original Message----- From: llvm-dev [mailto:llvm-dev-bounces at lists.llvm.org] On Behalf Of Liam Reimers via llvm-dev Sent: Thursday, January 10, 2019 02:21 To: David Greene <dag at cray.com> Cc: llvm-dev at lists.llvm.org Subject: Re: [llvm-dev] [llvm-rc] absolute.test failing I believe Windows uses slashes instead of hyphens for command line args. Liam> On Jan 9, 2019, at 2:03 PM, David Greene via llvm-dev <llvm-dev at lists.llvm.org> 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." > > 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 > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.llvm.org_cgi > -2Dbin_mailman_listinfo_llvm-2Ddev&d=DwIGaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r > =YgdxWMcdqQPlU9EdetI-xI79G7ouw9_Us0dFsZnFQYU&m=6ENmoSXIpoXe1OVAf8bSf39 > a8Sw4vpWeGCin4yUzb5g&s=tnu2WT7tKNeiDhu1mTtVYgIqngEZHT7gyUr8dYxM3Mc&e_______________________________________________ LLVM Developers mailing list llvm-dev at lists.llvm.org https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_llvm-2Ddev&d=DwIGaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=YgdxWMcdqQPlU9EdetI-xI79G7ouw9_Us0dFsZnFQYU&m=6ENmoSXIpoXe1OVAf8bSf39a8Sw4vpWeGCin4yUzb5g&s=tnu2WT7tKNeiDhu1mTtVYgIqngEZHT7gyUr8dYxM3Mc&e=
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>
David Greene via llvm-dev
2019-Jan-10 19:42 UTC
[llvm-dev] [llvm-rc] absolute.test failing
Martin Storsjö <martin at martin.st> writes:> 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?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