[llvm-ar] Add more tests for errors in opening archives

[sameerarora101]

Summary

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.

[MaskRay]

llvm/test/tools/llvm-ar/print.test

93

On some systems the diagnostic is is a directory

[sameerarora101]

llvm/test/tools/llvm-ar/print.test

93

Ok, I'll fix this. Thanks!

[sameerarora101]

Add lower case i for print test case

[jhenderson]

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.

[sameerarora101]

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?

[sameerarora101]

Add colon after the test title.

[sameerarora101]

@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

[sameerarora101]

fix old commit.

[gbreynoo]

In D80838#2065845, @jhenderson wrote:

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.

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.

[alexander-shaposhnikov]

I agree with @gbreynoo - i think it makes sense to place them separately.

[smeenai]

In D80838#2065845, @jhenderson wrote:

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.

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.

[jhenderson]

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.

[sameerarora101]

Moving tests to a new file under test/tools/llvm-ar.

[sameerarora101]

Thanks for the review everyone! I have now moved the tests to a new file test/tools/llvm-ar/error-opening-archive.test.

[sameerarora101]

Removing unnecessary RUN from error-opening-archive.test

[smeenai]

LGTM!

llvm/test/tools/llvm-ar/error-opening-archive.test

4–5

Nit: we don't need these two, right?

[jhenderson]

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

10–11

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.

[sameerarora101]

@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 :(

[sameerarora101]

Adding # UNSUPPORTED: system-windows to the test case.

[MaskRay]

LG, but you'll need to wait for @jhenderson to clear the blocker.

[jhenderson]

In D80838#2071252, @sameerarora101 wrote:

@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 :(

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

2

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.

[sameerarora101]

In D80838#2072913, @jhenderson wrote:

In D80838#2071252, @sameerarora101 wrote:

@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 :(

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.

Understood. Thanks a lot for clarifying!

[sameerarora101]

llvm/test/tools/llvm-ar/error-opening-archive.test

2

On it

[sameerarora101]

Separating permission and directory cases.

[jhenderson]

LGTM, with one suggestion.

llvm/test/tools/llvm-ar/error-opening-permission.test

2 ↗

(On Diff #268492)

I'd delete the "with chmod" bit of this comment.

[sameerarora101]

llvm/test/tools/llvm-ar/error-opening-permission.test

2 ↗

(On Diff #268492)

Got it

[sameerarora101]

Udating comment: removing "with chmod".

[gkistanova]

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

--

********************

[gkistanova]

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?

[sameerarora101]

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!

[jhenderson]

In D80838#2119065, @sameerarora101 wrote:

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!

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

[sameerarora101]

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!

[kevans]

In D80838#2119256, @jhenderson wrote:

In D80838#2119065, @sameerarora101 wrote:

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!

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

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.

[smeenai]

@sameerarora101, I think you should be able to mark this as closed manually, since the issues raised have been addressed.

[sameerarora101]

Closing revision as the issue has been addressed.

[jdelia]

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.

[jhenderson]

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?

llvm/test/tools/llvm-ar/error-opening-archive.test

+# RUN: rm -rf %t && mkdir -p %t
+# RUN: echo file1 > %t/1.txt
+# RUN: echo file2 > %t/2.txt
+# RUN: echo file3 > %t/3.txt
+
+# RUN: llvm-ar rc %t/archive.a %t/1.txt %t/2.txt %t/3.txt
+
+## No Permission:
+# RUN: llvm-ar rc %t/permission.b %t/1.txt
+# RUN: chmod 100 %t/permission.b
+# RUN: not llvm-ar p %t/permission.b 2>&1 \
+# RUN:   | FileCheck %s --check-prefix=NO-PERMISSION -DARCHIVE=%t/permission.b
+
+# NO-PERMISSION: error: error opening '[[ARCHIVE]]': Permission denied
+
+## Passing in a directory:
+# RUN: mkdir -p %t/tmpDir
+# RUN: not llvm-ar p %t/tmpDir 2>&1 \
+# RUN:    | FileCheck %s --check-prefix=IS-DIR -DARCHIVE=%t/tmpDir
+
+# IS-DIR: error: error opening '[[ARCHIVE]]': {{[iI]}}s a directory

llvm/test/tools/llvm-ar/print.test

 ## Print same member when containing multiple members with shared name:
 # llvm-ar -q %t/archive.a %t/2.txt
 # RUN: not llvm-ar p %t/archive.a %t/2.txt %t/2.txt 2>&1 \
 # RUN:   | FileCheck %s --check-prefix=SAME -DFILE=%t/2.txt
 
 ## No archive:
 # RUN: not llvm-ar p 2>&1 \
 # RUN:   | FileCheck %s --check-prefix=NO-ARCHIVE
-# 
+#
 # NO-ARCHIVE: error: an archive name must be specified
 
 ## Archive does not exist:
 # RUN: not llvm-ar p %t/missing.a 2>&1 \
 # RUN:   | FileCheck %s --check-prefix=MISSING-ARCHIVE -DARCHIVE=%t/missing.a
 
 # MISSING-ARCHIVE: error: error loading '[[ARCHIVE]]': {{[nN]}}o such file or directory
 
 ## Member does not exist:
 # RUN: not llvm-ar p %t/archive.a %t-missing.txt 2>&1 \
 # RUN:   | FileCheck %s --check-prefix=MISSING-FILE -DFILE=%t-missing.txt
 
 # MISSING-FILE: error: '[[FILE]]' was not found
 
 ## Print thin archive:
 # RUN: llvm-ar Trc %t/thin-archive.a %t/1.txt %t/2.txt %t/3.txt
 # RUN: llvm-ar p %t/archive.a %t/2.txt \
 # RUN:   | FileCheck %s --check-prefix=SINGLE --match-full-lines --implicit-check-not {{.}}