Add 2 more tests for the function
performOperation under llvm-ar.cpp. It tests
the scenario when the archive could not be opened
for reasons other than no_such_file_or_directory
In particular, it tests for the cases permission_denied
and is_a_directory for the target archive.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| llvm/test/tools/llvm-ar/print.test | ||
|---|---|---|
| 93 | On some systems the diagnostic is is a directory | |
| llvm/test/tools/llvm-ar/print.test | ||
|---|---|---|
| 93 | Ok, I'll fix this. Thanks! | |
Comment Actions
The test you have modified is for testing the "p" (print) operation, but your new test cases are not really about that option, but rather about generic file handling. I'm not convinced they belong in this test file, but @gbreynoo might have other opinions, since he's the one who's been working on the llvm-ar testing recently.
Could you clarify why you are adding these test cases? I don't think the behaviour is really part of llvm-ar, but rather part of the underlying support libraries which control when a file can be opened or not.
| llvm/test/tools/llvm-ar/print.test | ||
|---|---|---|
| 80 | Nit: add trailing colon to match the other comments. | |
| 81 | Traditional llvm-ar calling does not use - for its arguments. See the existing test cases. | |
| 82 | Will this do what is expected for Windows users? | |
| 88 | Ditto. | |
| llvm/test/tools/llvm-ar/print.test | ||
|---|---|---|
| 81 | calling llvm-ar with options rc was using a - as on line #9. Which one should I be following? | |
| 82 | The test case read-only-archive.test is also using a chmod command and is supported for all systems (the file does not has a UNSUPPORTED: system-windows to mark the test as unsupported on windows). Thus, it should be fine? | |
Comment Actions
@jhenderson Currently, there are no tests for the error opening ... and error loading ... cases in the function performOperation under llvm-ar.cpp. These new tests check these execution flows. I hope that's right, @smeenai ? Thanks
Comment Actions
I agree with James that these may be best placed in their own test. Currently there are individual tests for invalid-object-file.test, missing-thin-archive-member.test etc.
Comment Actions
We put them in this file because the "Archive does not exist" test was there, but yup, it'd make sense to move them out.
Could you clarify why you are adding these test cases? I don't think the behaviour is really part of llvm-ar, but rather part of the underlying support libraries which control when a file can be opened or not.
The logic to handle these errors is in llvm-ar.cpp (specifically one of the errors that's being modified in D80846), and it didn't have any test coverage before this.
| llvm/test/tools/llvm-ar/print.test | ||
|---|---|---|
| 81 | When you move this to the new file, go without the -. Sorry, I didn't spot that there were earlier cases with it, but I'd consider those less than ideal. | |
Comment Actions
Thanks for the review everyone! I have now moved the tests to a new file test/tools/llvm-ar/error-opening-archive.test.
Comment Actions
LGTM!
| llvm/test/tools/llvm-ar/error-opening-archive.test | ||
|---|---|---|
| 3–4 ↗ | (On Diff #267971) | Nit: we don't need these two, right? |
Comment Actions
According to the pre-merge checks, the new test is failing. Please review the logs and fix. It looks like the test may not work on some systems.
| llvm/test/tools/llvm-ar/error-opening-archive.test | ||
|---|---|---|
| 9–10 ↗ | (On Diff #267971) | Nit: my personal preference is for continuation lines to be formatted as follows: # RUN: not llvm-ar p %t/permission.b 2>&1 | \ # RUN: FileCheck %s --check-prefix=NO-PERMISSION -DARCHIVE=%t/permission.b The idea is that from the first line, it is clear that the next line is a new command, rather than just a continuation of the options for the instruction on that line, and from the second line, the extra indentation makes clear it is a continuation of the first line. Same applies below. |
Comment Actions
@jhenderson I think you were right. I'll try running the test case with # UNSUPPORTED: system-windows. However, I still don't understand why other test cases, for eg. read-only-archive.test or https://github.com/llvm/llvm-project/blob/master/lld/test/ELF/driver-access.test , are able to run on windows with chmod. Is the issue with the error msg? I checked the logs too but they aren't that helpful :(
Comment Actions
I can't say I fully understand it either, but I verified that it doesn't work on my Windows box either - it's actually successfully reading and printing the file contents. chmod does sort of work on Windows, but access permissions are different so it only is a rough approximation of the Unix equivalent. Thus, it is fairly trivial to mark things (to some degree at least) as read only (hence why those other tests work), but I'm guessing not as "unreadable". There's probably a way to do it, but not using chmod, and therefore unlikely to be in a portable fashion.
Arguably, the permission denied test isn't even needed in this case - the same code path at the tool level at least handles the "is directory" case too. I don't mind it being included though.
| llvm/test/tools/llvm-ar/error-opening-archive.test | ||
|---|---|---|
| 1 ↗ | (On Diff #268181) | Rather than making all cases unsupported on Windows, I think it would be preferable to split this test into two, with the permission bit in its own test. You should also add a comment to that case saying why it is unsupported on Windows. |
| llvm/test/tools/llvm-ar/error-opening-archive.test | ||
|---|---|---|
| 1 ↗ | (On Diff #268181) | On it |
Comment Actions
LGTM, with one suggestion.
| llvm/test/tools/llvm-ar/error-opening-permission.test | ||
|---|---|---|
| 2 | I'd delete the "with chmod" bit of this comment. | |
| llvm/test/tools/llvm-ar/error-opening-permission.test | ||
|---|---|---|
| 2 | Got it | |
Comment Actions
This patch introduced a test failure on FreeBSD.
The error message is different than what's the expected by the test.
******************** TEST 'LLVM :: tools/llvm-ar/error-opening-directory.test' FAILED ********************
Script:
--
: 'RUN: at line 1'; rm -rf /usr/home/buildbot/as-bldslv5/lld-x86_64-freebsd/build/test/tools/llvm-ar/Output/error-opening-directory.test.tmp && mkdir -p /usr/home/buildbot/as-bldslv5/lld-x86_64-freebsd/build/test/tools/llvm-ar/Output/error-opening-directory.test.tmp
: 'RUN: at line 4'; mkdir -p /usr/home/buildbot/as-bldslv5/lld-x86_64-freebsd/build/test/tools/llvm-ar/Output/error-opening-directory.test.tmp/tmpDir
: 'RUN: at line 5'; not /home/buildbot/as-bldslv5/lld-x86_64-freebsd/build/bin/llvm-ar p /usr/home/buildbot/as-bldslv5/lld-x86_64-freebsd/build/test/tools/llvm-ar/Output/error-opening-directory.test.tmp/tmpDir 2>&1 | /home/buildbot/as-bldslv5/lld-x86_64-freebsd/build/bin/FileCheck /usr/home/buildbot/as-bldslv5/lld-x86_64-freebsd/llvm-project/llvm/test/tools/llvm-ar/error-opening-directory.test --check-prefix=IS-DIR -DARCHIVE=/usr/home/buildbot/as-bldslv5/lld-x86_64-freebsd/build/test/tools/llvm-ar/Output/error-opening-directory.test.tmp/tmpDir
--
Exit Code: 1
Command Output (stderr):
--
+ : 'RUN: at line 1'
+ rm -rf /usr/home/buildbot/as-bldslv5/lld-x86_64-freebsd/build/test/tools/llvm-ar/Output/error-opening-directory.test.tmp
+ mkdir -p /usr/home/buildbot/as-bldslv5/lld-x86_64-freebsd/build/test/tools/llvm-ar/Output/error-opening-directory.test.tmp
+ : 'RUN: at line 4'
+ mkdir -p /usr/home/buildbot/as-bldslv5/lld-x86_64-freebsd/build/test/tools/llvm-ar/Output/error-opening-directory.test.tmp/tmpDir
+ : 'RUN: at line 5'
+ not /home/buildbot/as-bldslv5/lld-x86_64-freebsd/build/bin/llvm-ar p /usr/home/buildbot/as-bldslv5/lld-x86_64-freebsd/build/test/tools/llvm-ar/Output/error-opening-directory.test.tmp/tmpDir
+ /home/buildbot/as-bldslv5/lld-x86_64-freebsd/build/bin/FileCheck /usr/home/buildbot/as-bldslv5/lld-x86_64-freebsd/llvm-project/llvm/test/tools/llvm-ar/error-opening-directory.test --check-prefix=IS-DIR -DARCHIVE=/usr/home/buildbot/as-bldslv5/lld-x86_64-freebsd/build/test/tools/llvm-ar/Output/error-opening-directory.test.tmp/tmpDir
/usr/home/buildbot/as-bldslv5/lld-x86_64-freebsd/llvm-project/llvm/test/tools/llvm-ar/error-opening-directory.test:8:11: error: IS-DIR: expected string not found in input
# IS-DIR: error: unable to open '[[ARCHIVE]]': {{[iI]}}s a directory
^
<stdin>:1:1: note: scanning from here
/home/buildbot/as-bldslv5/lld-x86_64-freebsd/build/bin/llvm-ar: error: unable to load '/usr/home/buildbot/as-bldslv5/lld-x86_64-freebsd/build/test/tools/llvm-ar/Output/error-opening-directory.test.tmp/tmpDir': file too small to be an archive
^
<stdin>:1:1: note: with "ARCHIVE" equal to "/usr/home/buildbot/as-bldslv5/lld-x86_64-freebsd/build/test/tools/llvm-ar/Output/error-opening-directory\\.test\\.tmp/tmpDir"
/home/buildbot/as-bldslv5/lld-x86_64-freebsd/build/bin/llvm-ar: error: unable to load '/usr/home/buildbot/as-bldslv5/lld-x86_64-freebsd/build/test/tools/llvm-ar/Output/error-opening-directory.test.tmp/tmpDir': file too small to be an archive
^
<stdin>:1:65: note: possible intended match here
/home/buildbot/as-bldslv5/lld-x86_64-freebsd/build/bin/llvm-ar: error: unable to load '/usr/home/buildbot/as-bldslv5/lld-x86_64-freebsd/build/test/tools/llvm-ar/Output/error-opening-directory.test.tmp/tmpDir': file too small to be an archive
^
Input file: <stdin>
Check file: /usr/home/buildbot/as-bldslv5/lld-x86_64-freebsd/llvm-project/llvm/test/tools/llvm-ar/error-opening-directory.test
-dump-input=help describes the format of the following dump.
Full input was:
<<<<<<
1: /home/buildbot/as-bldslv5/lld-x86_64-freebsd/build/bin/llvm-ar: error: unable to load '/usr/home/buildbot/as-bldslv5/lld-x86_64-freebsd/build/test/tools/llvm-ar/Output/error-opening-directory.test.tmp/tmpDir': file too small to be an archive
check:8'0 X~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found
check:8'1 ? possible intended match
>>>>>>
--
********************Comment Actions
Hi Sameer,
Could you either teach the error-opening-directory.test that adirectory could be opened for reading just fine, thus the error message would be different than what you expect here, or handle that in llvm-ar to make sure the error message is reliable, please?
Comment Actions
Seems like FreeBSD allows for reading directories. In that case would the issue be resolved in case I add the following line to the test:
# UNSUPPORTED: system-freebsd
or perhaps
# XFAIL: system-freebsd
I'll update the test accordingly then. Thanks a lot!
Comment Actions
# UNSUPPORTED would be the right approach - I don't think the FreeBSD behaviour is something we're likely to "fix" either in FreeBSD or llvm-ar so that the test will pass. I don't know if freebsd is a "system" that can easily be identified though.
Comment Actions
Ok, so I added # UNSUPPORTED: system-freebsd. Here is the patch https://reviews.llvm.org/D82786. Please lemme know if this doesn't resolve the issue. Thanks!
Comment Actions
I mentioned this in the other review, but for what it's worth- we have in-fact managed to change this behavior for FreeBSD 13.0-CURRENT, and I expect to make the change for FreeBSD 12.2 if it doesn't cause too much friction. UNSUPPORTED still seems correct, since you'll have a mixed bag of pass/fail based on exact FreeBSD version (and filesystem, even) in play.
Comment Actions
@sameerarora101, I think you should be able to mark this as closed manually, since the issues raised have been addressed.
Comment Actions
Hi. I have an error when building LLVM 11.0.0 on a laptop (Intel(R) Core(TM) i7-2630QM CPU @ 2.00GHz) with Fedora 33 (kernel-core-5.9.13-200.fc33). The full history is at https://llvm.discourse.group/t/error-when-building-flang-with-llvm-11-0-0/2394. When I do the llvm full tests (with make check-all) there are some errors. The first one is:
/home/bigpack/llvm-paq/11.0.0/llvm-project/llvm/test/tools/llvm-ar/error-opening-permission.test:14:18: error: NO-PERMISSION: expected string not found in input :1:1: note: with “ARCHIVE” equal to “/home/bigpack/llvm-paq/11.0.0/llvm-project/build/test/tools/llvm-ar/Output/error-opening-permission.test.tmp/permission.b”
It looks FileCheck’s ARCHIVE varible is not correctly set because of 11.0.0 string in directory path. So FileCheck unable to find ‘error: unable to open ‘[[ARCHIVE]]’: {{[pP]}}ermission denied’ on the output of command line(insted seeing file not found). It may be bug in Fileckeck. Then, I tried a single test, but also is failed:
cd /home/bigpack/llvm-paq/11.0.0/llvm-project/build/
$ bin/llvm-lit …/llvm/test/tools/llvm-ar/error-opening-permission.test
– Testing: 1 tests, 1 workers –
FAIL: LLVM :: tools/llvm-ar/error-opening-permission.test (1 of 1)
Failed Tests (1):
LLVM :: tools/llvm-ar/error-opening-permission.test
Testing Time: 0.16s
Failed: 1
Then, a developer of the llvm team (Shivam) wrote:
I found the patch that introduces this test. The test that failing is unsupported on windows and maybe on your system also.
Here is the patch https://reviews.llvm.org/D80838. I think you should ping @sameerarora101 on Phabricator & explain the issue.
Then, please, some clue for fix this problem? Regards.
Comment Actions
Hi @jdelia,
Sorry for the delay in responding. I and I suspect others who were involved on this have been off over the Christmas period.
I'm not sure why you think ARCHIVE is being incorrectly set. To me, it looks like it is being set exactly to what it should be. From line 12 of the failing test, you have -DARCHIVE=%t/permission.b. lit will expand %t to an absolute path pointing to a name unique to the test. In this case, it is /home/bigpack/llvm-paq/11.0.0/llvm-project/build/test/tools/llvm-ar/Output/error-opening-permission.test.tmp, and then a /permission.b is concatenated to the end of the string to form the value of ARCHIVE. FileCheck will then look for the line error: error opening '/home/bigpack/llvm-paq/11.0.0/llvm-project/build/test/tools/llvm-ar/Output/error-opening-permission.test.tmp/permission.b': permission denied (where the 'p' in 'permission' can be upper of lower-case) in the output of not llvm-ar p /home/bigpack/llvm-paq/11.0.0/llvm-project/build/test/tools/llvm-ar/Output/error-opening-permission.test.tmp/permission.b (stderr or stdout).
What happens when you try to manually run the llvm-ar command locally on the permission.b path? What output do you see (if any)? Does llvm-ar pass or fail? Also, what system are you running on? As noted in the test comments, the test doesn't work on Windows due to the difficulty of making a file unreadable on that platform. Maybe your platform has a similar problem?
I'd delete the "with chmod" bit of this comment.