Page MenuHomePhabricator

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

Authored by sameerarora101 on May 29 2020, 1:54 PM.

Details

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.

Diff Detail

Event Timeline

sameerarora101 created this revision.May 29 2020, 1:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 29 2020, 1:55 PM
MaskRay added inline comments.May 29 2020, 2:30 PM
llvm/test/tools/llvm-ar/print.test
93 ↗(On Diff #267363)

On some systems the diagnostic is is a directory

sameerarora101 marked an inline comment as done.May 29 2020, 3:14 PM
sameerarora101 added inline comments.
llvm/test/tools/llvm-ar/print.test
93 ↗(On Diff #267363)

Ok, I'll fix this. Thanks!

Add lower case i for print test case

jhenderson added a subscriber: gbreynoo.

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 ↗(On Diff #267383)

Nit: add trailing colon to match the other comments.

81 ↗(On Diff #267383)

Traditional llvm-ar calling does not use - for its arguments. See the existing test cases.

82 ↗(On Diff #267383)

Will this do what is expected for Windows users?

88 ↗(On Diff #267383)

Ditto.

sameerarora101 marked 4 inline comments as done.Jun 1 2020, 6:42 AM
sameerarora101 added inline comments.
llvm/test/tools/llvm-ar/print.test
81 ↗(On Diff #267383)

calling llvm-ar with options rc was using a - as on line #9. Which one should I be following?

82 ↗(On Diff #267383)

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?

Add colon after the test title.

@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

fix old commit.

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.

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

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 added inline comments.Jun 2 2020, 1:21 AM
llvm/test/tools/llvm-ar/print.test
81 ↗(On Diff #267383)

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 updated this revision to Diff 267966.EditedJun 2 2020, 1:14 PM

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

sameerarora101 added a comment.EditedJun 2 2020, 1:15 PM

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

Removing unnecessary RUN from error-opening-archive.test

smeenai accepted this revision.Jun 2 2020, 5:03 PM

LGTM!

llvm/test/tools/llvm-ar/error-opening-archive.test
3–4 ↗(On Diff #267971)

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

This revision is now accepted and ready to land.Jun 2 2020, 5:03 PM
alexshap accepted this revision.Jun 2 2020, 5:26 PM
jhenderson requested changes to this revision.Jun 3 2020, 12:14 AM

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.

This revision now requires changes to proceed.Jun 3 2020, 12:14 AM
sameerarora101 marked 2 inline comments as done.Jun 3 2020, 6:51 AM

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

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

MaskRay accepted this revision.Jun 3 2020, 12:46 PM

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

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

@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 marked an inline comment as done.Jun 4 2020, 6:19 AM
sameerarora101 added inline comments.
llvm/test/tools/llvm-ar/error-opening-archive.test
1 ↗(On Diff #268181)

On it

Separating permission and directory cases.

jhenderson accepted this revision.Jun 4 2020, 11:59 PM

LGTM, with one suggestion.

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

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

This revision is now accepted and ready to land.Jun 4 2020, 11:59 PM
sameerarora101 marked 2 inline comments as done.Jun 5 2020, 6:36 AM
sameerarora101 added inline comments.
llvm/test/tools/llvm-ar/error-opening-permission.test
3

Got it

sameerarora101 marked an inline comment as done.

Udating comment: removing "with chmod".

jhenderson accepted this revision.Jun 5 2020, 7:40 AM
This revision was automatically updated to reflect the committed changes.
gkistanova reopened this revision.Jun 27 2020, 12:41 AM
gkistanova added a subscriber: 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
>>>>>>

--

********************
This revision is now accepted and ready to land.Jun 27 2020, 12:41 AM
gkistanova requested changes to this revision.Jun 27 2020, 12:49 AM

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?

This revision now requires changes to proceed.Jun 27 2020, 12:49 AM

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!

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.

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 added a subscriber: kevans.EditedJun 29 2020, 11:12 AM

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.

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

sameerarora101 abandoned this revision.Tue, Aug 4, 2:45 PM

Closing revision as the issue has been addressed.