This is an archive of the discontinued LLVM Phabricator instance.

Differential D77659

[ELF][test] Add reproduce test for dependent libraries
ClosedPublic

Authored by andrewng on Apr 7 2020, 9:32 AM.

Details

Reviewers
ruiu
grimar
MaskRay
• espindola
Commits
rG3db215089f48: [ELF][test] Add reproduce test for dependent libraries

Diff Detail

Event Timeline

andrewng created this revision.Apr 7 2020, 9:32 AM
Herald added a reviewer: • espindola. · View Herald TranscriptApr 7 2020, 9:32 AM
Herald added a subscriber: emaste. · View Herald Transcript
MaskRay added a comment.Apr 7 2020, 10:38 AM

Thanks. Probably add [test] to the title because there is no code change.

lld/test/ELF/reproduce-deplibs.s
12

-o /dev/null

14

cmp may be better for comparison between binary files.

andrewng updated this revision to Diff 255736.Apr 7 2020, 11:04 AM
andrewng retitled this revision from [ELF] Add reproduce test for dependent libraries to [ELF][test] Add reproduce test for dependent libraries.

Updated to address review comments.

andrewng marked 2 inline comments as done.Apr 7 2020, 11:10 AM
andrewng added inline comments.
lld/test/ELF/reproduce-deplibs.s
14

The other reproduce tests make use of diff, so I've just followed their lead. However, cmp probably is more appropriate for the binary files. Perhaps we should change them all to use cmp instead of diff where applicable?

MaskRay added inline comments.Apr 7 2020, 12:04 PM
lld/test/ELF/reproduce-deplibs.s
2

Can you also delete shell and check whether harbomaster (pre-merge testing) complains?

14

Yeah, I've noticed that. diff -> cmp should be fine.

Out of curiosity, how did you notice that reproduce-deplibs.s should be tested as well?

andrewng marked 2 inline comments as done.Apr 7 2020, 2:02 PM
andrewng added inline comments.
lld/test/ELF/reproduce-deplibs.s
2

I can try deleting shell but will need to add it back to prevent potential testing issues on Windows related to paths being too long when the reproduce TAR file is extracted. That's the reason why all the reproduce tests that extract the TAR file include shell.

14

I will change to cmp, although diff is a lit built-in command, so that might explain its usage elsewhere.

We have some legacy downstream regression tests which we are gradually migrating to lit. That's why we noticed that this coverage was missing.

andrewng updated this revision to Diff 255800.Apr 7 2020, 2:05 PM

Updated to address review comments.

MaskRay accepted this revision.Apr 7 2020, 2:40 PM

I don't know much about lit internals but the existing REQUIRES: shell made me wonder why it is needed. Feel free to keep it or not at your discretion.

This revision is now accepted and ready to land.Apr 7 2020, 2:40 PM
ruiu added inline comments.Apr 7 2020, 9:31 PM
lld/test/ELF/reproduce-deplibs.s
2

So basically you want to run this test on non-windows machine right?

UNSUPPORTED: system-windows

should express the intention more clearly.

jhenderson added inline comments.Apr 8 2020, 1:08 AM
lld/test/ELF/reproduce-deplibs.s
2

It might be worth a comment too saying WHY it's not supported on Windows, because that restriction isn't obvious at first glance.

13

Idle musing, feel free to ignore: I wonder whether we really need to check that the archive in the tar file is binary identical to the input one? Would just checking that the file is in the package be sufficient?

grimar added inline comments.Apr 8 2020, 1:48 AM
lld/test/ELF/reproduce-deplibs.s
13

I also wonder about it.

Our reproduce-windows.s has:

# RUN: tar tf repro.tar | FileCheck %s

# CHECK: repro/response.txt
# CHECK: repro/{{.*}}/build/foo.o
andrewng marked 2 inline comments as done.Apr 8 2020, 2:24 AM
andrewng added inline comments.
lld/test/ELF/reproduce-deplibs.s
2

I've followed what the other reproduce tests have done. I suspect the reasoning for shell is that this can be overridden if your Windows shell, environment and tools can support long paths. By default, shell is not enabled on Windows.

I will add a comment.

13

I've basically followed the convention of the other reproduce tests (except the Windows specific ones) that all check the contents too. I'm guessing this is just to be sure that it's also the correct file. I'm happy to change it to only test for its presence in the TAR file if that's the consensus.

jhenderson added inline comments.Apr 8 2020, 2:49 AM
lld/test/ELF/reproduce-deplibs.s
13

I don't really have an opinion either way. Happy to defer to others on this one.

andrewng updated this revision to Diff 255942.Apr 8 2020, 3:03 AM

Add comment to explain the Windows deficiencies (one of many)...

grimar added inline comments.Apr 8 2020, 3:17 AM
lld/test/ELF/reproduce-deplibs.s
6

Does it mean that using tar tf like reproduce-windows.s do would allow this to be enabled on windows?

13

I'd slightly prefer to avoid checking the content because LLD tests are already take some time to pass and would be good to reduce this time. I do not mind to leave it for a follow-up, but see my question above.

andrewng marked 2 inline comments as done.Apr 8 2020, 4:01 AM
andrewng added inline comments.
lld/test/ELF/reproduce-deplibs.s
6

Yes, tar tf only lists the contents of the TAR archive. That's why that is used in the Windows tests.

13

I think that if these comparisons are a concern, then we should address all the reproduce tests accordingly in a follow up.

grimar accepted this revision.Apr 8 2020, 4:29 AM

LGTM

lld/test/ELF/reproduce-deplibs.s
6

Then it is better to do in this way as disabling this test is unnecessary. If your intention is to improve testing coverage then enabling it to run on windows bots looks like a good thing to me.

13

My main concern now it that this and seems few other tests about --reproduce are disabled for no reason it seems.
Addressing the reproduce tests accordingly would solve both problems at once.

Closed by commit rG3db215089f48: [ELF][test] Add reproduce test for dependent libraries (authored by andrewng). · Explain WhyApr 8 2020, 5:24 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 8 2020, 5:24 AM
andrewng mentioned this in D77750: [ELF][test] Improve reproduce tests and enable for Windows.Apr 8 2020, 12:40 PM
andrewng mentioned this in rGd08105482e15: [ELF][test] Improve reproduce tests and enable for Windows.Apr 9 2020, 8:08 AM

Revision Contents

PathSize
lld/
test/
ELF/
23 lines

Diff 255985

lld/test/ELF/reproduce-deplibs.s

  • This file was added.
# REQUIRES: x86, shell
MaskRayUnsubmitted
Not Done
ReplyInline Actions

Can you also delete shell and check whether harbomaster (pre-merge testing) complains?

MaskRay: Can you also delete `shell` and check whether harbomaster (pre-merge testing) complains?
andrewngAuthorUnsubmitted
Done
ReplyInline Actions

I can try deleting shell but will need to add it back to prevent potential testing issues on Windows related to paths being too long when the reproduce TAR file is extracted. That's the reason why all the reproduce tests that extract the TAR file include shell.

andrewng: I can try deleting `shell` but will need to add it back to prevent potential testing issues on…
ruiuUnsubmitted
Not Done
ReplyInline Actions

So basically you want to run this test on non-windows machine right?

UNSUPPORTED: system-windows

should express the intention more clearly.

ruiu: So basically you want to run this test on non-windows machine right? UNSUPPORTED: system…
jhendersonUnsubmitted
Not Done
ReplyInline Actions

It might be worth a comment too saying WHY it's not supported on Windows, because that restriction isn't obvious at first glance.

jhenderson: It might be worth a comment too saying WHY it's not supported on Windows, because that…
andrewngAuthorUnsubmitted
Done
ReplyInline Actions

I've followed what the other reproduce tests have done. I suspect the reasoning for shell is that this can be overridden if your Windows shell, environment and tools can support long paths. By default, shell is not enabled on Windows.

I will add a comment.

andrewng: I've followed what the other reproduce tests have done. I suspect the reasoning for `shell` is…
## The 'shell' requirement is to prevent this test from running by default on
## Windows as the extraction of the tar archive can cause problems related to
## path length limits.
grimarUnsubmitted
Not Done
ReplyInline Actions

Does it mean that using tar tf like reproduce-windows.s do would allow this to be enabled on windows?

grimar: Does it mean that using `tar tf` like `reproduce-windows.s` do would allow this to be enabled…
andrewngAuthorUnsubmitted
Done
ReplyInline Actions

Yes, tar tf only lists the contents of the TAR archive. That's why that is used in the Windows tests.

andrewng: Yes, `tar tf` only lists the contents of the TAR archive. That's why that is used in the…
grimarUnsubmitted
Not Done
ReplyInline Actions

Then it is better to do in this way as disabling this test is unnecessary. If your intention is to improve testing coverage then enabling it to run on windows bots looks like a good thing to me.

grimar: Then it is better to do in this way as disabling this test is unnecessary. If your intention is…
# RUN: rm -rf %t.dir
# RUN: mkdir -p %t.dir
# RUN: cd %t.dir
# RUN: echo ".global foo; foo: ret" | \
# RUN: llvm-mc -filetype=obj -triple=x86_64 - -o foo.o
# RUN: llvm-ar rc foo.a foo.o
MaskRayUnsubmitted
Done
ReplyInline Actions

-o /dev/null

MaskRay: `-o /dev/null`
# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o bar.o
jhendersonUnsubmitted
Not Done
ReplyInline Actions

Idle musing, feel free to ignore: I wonder whether we really need to check that the archive in the tar file is binary identical to the input one? Would just checking that the file is in the package be sufficient?

jhenderson: Idle musing, feel free to ignore: I wonder whether we really need to check that the archive in…
grimarUnsubmitted
Not Done
ReplyInline Actions

I also wonder about it.

Our reproduce-windows.s has:

# RUN: tar tf repro.tar | FileCheck %s

# CHECK: repro/response.txt
# CHECK: repro/{{.*}}/build/foo.o
grimar: I also wonder about it. Our `reproduce-windows.s` has: ``` # RUN: tar tf repro.tar | FileCheck…
andrewngAuthorUnsubmitted
Done
ReplyInline Actions

I've basically followed the convention of the other reproduce tests (except the Windows specific ones) that all check the contents too. I'm guessing this is just to be sure that it's also the correct file. I'm happy to change it to only test for its presence in the TAR file if that's the consensus.

andrewng: I've basically followed the convention of the other reproduce tests (except the Windows…
jhendersonUnsubmitted
Not Done
ReplyInline Actions

I don't really have an opinion either way. Happy to defer to others on this one.

jhenderson: I don't really have an opinion either way. Happy to defer to others on this one.
grimarUnsubmitted
Not Done
ReplyInline Actions

I'd slightly prefer to avoid checking the content because LLD tests are already take some time to pass and would be good to reduce this time. I do not mind to leave it for a follow-up, but see my question above.

grimar: I'd slightly prefer to avoid checking the content because LLD tests are already take some time…
andrewngAuthorUnsubmitted
Done
ReplyInline Actions

I think that if these comparisons are a concern, then we should address all the reproduce tests accordingly in a follow up.

andrewng: I think that if these comparisons are a concern, then we should address all the reproduce tests…
grimarUnsubmitted
Not Done
ReplyInline Actions

My main concern now it that this and seems few other tests about --reproduce are disabled for no reason it seems.
Addressing the reproduce tests accordingly would solve both problems at once.

grimar: My main concern now it that this and seems few other tests about `--reproduce` are disabled for…
MaskRayUnsubmitted
Not Done
ReplyInline Actions

cmp may be better for comparison between binary files.

MaskRay: `cmp` may be better for comparison between binary files.
andrewngAuthorUnsubmitted
Done
ReplyInline Actions

The other reproduce tests make use of diff, so I've just followed their lead. However, cmp probably is more appropriate for the binary files. Perhaps we should change them all to use cmp instead of diff where applicable?

andrewng: The other reproduce tests make use of `diff`, so I've just followed their lead. However, `cmp`…
MaskRayUnsubmitted
Not Done
ReplyInline Actions

Yeah, I've noticed that. diff -> cmp should be fine.

Out of curiosity, how did you notice that reproduce-deplibs.s should be tested as well?

MaskRay: Yeah, I've noticed that. `diff -> cmp` should be fine. Out of curiosity, how did you notice…
andrewngAuthorUnsubmitted
Done
ReplyInline Actions

I will change to cmp, although diff is a lit built-in command, so that might explain its usage elsewhere.

We have some legacy downstream regression tests which we are gradually migrating to lit. That's why we noticed that this coverage was missing.

andrewng: I will change to `cmp`, although `diff` is a lit built-in command, so that might explain its…
# RUN: ld.lld bar.o -o /dev/null --reproduce repro.tar
# RUN: tar xf repro.tar
# RUN: cmp foo.a repro/%:t.dir/foo.a
.globl _start
_start:
call foo
.section ".deplibs","MS",@llvm_dependent_libraries,1
.asciz "foo.a"