This is an archive of the discontinued LLVM Phabricator instance.

[llvm-ar][test] Increase llvm-ar test coverage
ClosedPublic

Authored by gbreynoo on Jun 28 2019, 8:28 AM.

Diff Detail

Event Timeline

gbreynoo created this revision.Jun 28 2019, 8:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 28 2019, 8:28 AM
MaskRay added inline comments.Jun 29 2019, 6:42 AM
test/tools/llvm-ar/create.test
2

Since you've used ## below..

# -> ##

test/tools/llvm-ar/dash-before-letter.test
2

# -> ##

test/tools/llvm-ar/extract.test
2

# -> ##

8

--implicit-check-not {{.}} does not check a regular expression.

Use | count 0 instead

test/tools/llvm-ar/insert-after.test
2

# -> ##

4

Maybe split this into rm %t-back.a/etc below.

gbreynoo updated this revision to Diff 207340.Jul 1 2019, 9:04 AM
gbreynoo marked 2 inline comments as done.Jul 1 2019, 9:15 AM

After MaskRay's suggestion I have removed all use of rm -f %t-*

test/tools/llvm-ar/create.test
2

The times I've used ## below has been to make comments stand out more in files that require the use of # on each line. Would it be preferred to standardise the use of ## and # on all files regardless of their use as input YAML files?

test/tools/llvm-ar/extract.test
8

I did some experimentation with implicit-check-not and a regular expression and it seemed to work fine, do you have a test case that illustrates the issue?

MaskRay added inline comments.Jul 3 2019, 11:31 PM
test/tools/llvm-ar/create.test
2

From what I can tell in llvm-objcopy, llvm-objdump, ..., comments in the header are also prefixed with ## nowadays.

9

No strong preference here, but if you like it:

RUN: rm -f %t.warning.ar
RUN: llvm-ar r %t.warning.ar %t1.txt %t2.txt 2>&1 \
RUN:   | FileCheck %s --check-prefix=WARNING -DOUTPUT=%t.warning.ar

WARNING: warning: creating [[OUTPUT]]
13

FileCheck --allow-empty /dev/null --implicit-check-not={{.}}

test/tools/llvm-ar/dash-before-letter.test
2

Can you merge this test with default-add.test ?

test/tools/llvm-ar/extract.test
8

Thanks for clarification. It works.

FileCheck --allow-empty /dev/null --implicit-check-not={{.}}

14

test/tools/llvm-ar/move-after.test
32

It may be interesting to specifies files not in the order:

llvm-ar ma %t1.txt %t-multiple.a %t4.txt %t3.txt

test/tools/llvm-ar/preserve-mod-time.test
7 ↗(On Diff #207340)

TZ=UTC

10 ↗(On Diff #207340)

--full-time is specific to GNU coreutils.

See llvm-objcopy/ELF/strip-preserve-mtime.test, check just the yyyy part should be ok:

# CHECK-PRESERVE-MTIME:        {{[[:space:]]1997}}
gbreynoo updated this revision to Diff 208050.Jul 4 2019, 8:22 AM

Thanks for the comments MaskRay, I have updated the revision.

gbreynoo marked an inline comment as done.Jul 4 2019, 8:26 AM
gbreynoo added inline comments.
test/tools/llvm-ar/dash-before-letter.test
2

I didn't realise that default-add.test already covered this functionality. Unless we want to spin off a new test explicitly for dashes before letters then I think default-add.test should suffice for coverage.

MaskRay added inline comments.Jul 4 2019, 7:46 PM
test/tools/llvm-ar/dash-before-letter.test
2

The spin-off dash-before-letter.test may be clearer. You can probably delete the multi-dash tests from default-add.test.

gbreynoo updated this revision to Diff 208124.Jul 5 2019, 12:36 AM
MaskRay accepted this revision.Jul 5 2019, 1:01 AM
MaskRay added inline comments.
test/tools/llvm-ar/extract.test
4

##

test/tools/llvm-ar/insert-after.test
18

##

test/tools/llvm-ar/insert-before.test
8

##

test/tools/llvm-ar/move-before.test
8

##

This revision is now accepted and ready to land.Jul 5 2019, 1:01 AM
This revision was automatically updated to reflect the committed changes.
phosek added a subscriber: phosek.Jul 8 2019, 12:35 PM

The tools/llvm-ar/extract.test is failing on macOS:

******************** TEST 'LLVM :: tools/llvm-ar/extract.test' FAILED ********************
Script:
--
: 'RUN: at line 2';   rm -rf /b/s/w/ir/k/recipe_cleanup/clanglRrIXg/llvm_build_dir/test/tools/llvm-ar/Output/extract.test.tmp && mkdir -p /b/s/w/ir/k/recipe_cleanup/clanglRrIXg/llvm_build_dir/test/tools/llvm-ar/Output/extract.test.tmp/extracted/
: 'RUN: at line 5';   /b/s/w/ir/k/recipe_cleanup/clanglRrIXg/llvm_build_dir/bin/llvm-ar cr /b/s/w/ir/k/recipe_cleanup/clanglRrIXg/llvm_build_dir/test/tools/llvm-ar/Output/extract.test.tmp/empty.a
: 'RUN: at line 6';   /b/s/w/ir/k/recipe_cleanup/clanglRrIXg/llvm_build_dir/bin/llvm-ar x /b/s/w/ir/k/recipe_cleanup/clanglRrIXg/llvm_build_dir/test/tools/llvm-ar/Output/extract.test.tmp/empty.a 2>&1    | /b/s/w/ir/k/recipe_cleanup/clanglRrIXg/llvm_build_dir/bin/FileCheck --allow-empty /dev/null --implicit-check-not={{.}}
: 'RUN: at line 9';   echo filea > /b/s/w/ir/k/recipe_cleanup/clanglRrIXg/llvm_build_dir/test/tools/llvm-ar/Output/extract.test.tmp/a.txt
: 'RUN: at line 10';   echo fileb > /b/s/w/ir/k/recipe_cleanup/clanglRrIXg/llvm_build_dir/test/tools/llvm-ar/Output/extract.test.tmp/b.txt
: 'RUN: at line 11';   /b/s/w/ir/k/recipe_cleanup/clanglRrIXg/llvm_build_dir/bin/llvm-ar rc /b/s/w/ir/k/recipe_cleanup/clanglRrIXg/llvm_build_dir/test/tools/llvm-ar/Output/extract.test.tmp/archive.a /b/s/w/ir/k/recipe_cleanup/clanglRrIXg/llvm_build_dir/test/tools/llvm-ar/Output/extract.test.tmp/a.txt /b/s/w/ir/k/recipe_cleanup/clanglRrIXg/llvm_build_dir/test/tools/llvm-ar/Output/extract.test.tmp/b.txt
: 'RUN: at line 14';   cd /b/s/w/ir/k/recipe_cleanup/clanglRrIXg/llvm_build_dir/test/tools/llvm-ar/Output/extract.test.tmp/extracted && /b/s/w/ir/k/recipe_cleanup/clanglRrIXg/llvm_build_dir/bin/llvm-ar x /b/s/w/ir/k/recipe_cleanup/clanglRrIXg/llvm_build_dir/test/tools/llvm-ar/Output/extract.test.tmp/archive.a a.txt
: 'RUN: at line 15';   diff /b/s/w/ir/k/recipe_cleanup/clanglRrIXg/llvm_build_dir/test/tools/llvm-ar/Output/extract.test.tmp/a.txt /b/s/w/ir/k/recipe_cleanup/clanglRrIXg/llvm_build_dir/test/tools/llvm-ar/Output/extract.test.tmp/extracted/a.txt
: 'RUN: at line 18';   rm /b/s/w/ir/k/recipe_cleanup/clanglRrIXg/llvm_build_dir/test/tools/llvm-ar/Output/extract.test.tmp/extracted/a.txt
: 'RUN: at line 19';   cd /b/s/w/ir/k/recipe_cleanup/clanglRrIXg/llvm_build_dir/test/tools/llvm-ar/Output/extract.test.tmp/extracted && /b/s/w/ir/k/recipe_cleanup/clanglRrIXg/llvm_build_dir/bin/llvm-ar x /b/s/w/ir/k/recipe_cleanup/clanglRrIXg/llvm_build_dir/test/tools/llvm-ar/Output/extract.test.tmp/archive.a
: 'RUN: at line 20';   diff /b/s/w/ir/k/recipe_cleanup/clanglRrIXg/llvm_build_dir/test/tools/llvm-ar/Output/extract.test.tmp/a.txt /b/s/w/ir/k/recipe_cleanup/clanglRrIXg/llvm_build_dir/test/tools/llvm-ar/Output/extract.test.tmp/extracted/a.txt
: 'RUN: at line 21';   diff /b/s/w/ir/k/recipe_cleanup/clanglRrIXg/llvm_build_dir/test/tools/llvm-ar/Output/extract.test.tmp/b.txt /b/s/w/ir/k/recipe_cleanup/clanglRrIXg/llvm_build_dir/test/tools/llvm-ar/Output/extract.test.tmp/extracted/b.txt
--
Exit Code: 1

Command Output (stdout):
--
1a2,3
> 
> 

--

Is it possible to revert this change?

Thanks phosek and jfb, I will investigate this macOS issue.

I'd say XFAIL: darwin would be cleaner then a revert... This apparently works fine on other OSes except that there is some hidden macOS issue that hasn't been sorted out.

If the archive member is not a regular object, the format will be detected incorrectly?

Simmilarly to D63197, I'm not sure if it's preferable to make this test XFAIL: darwin or explicitly call llvm-ar with --format=gnu. What do you think jfb?