This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objdump][test] Fix --prefix tests for system-windows
ClosedPublic

Authored by tinti on Jan 27 2021, 2:14 AM.

Details

Summary

[llvm-objdump][test] Fix --prefix tests for system-windows

Merging directories and files may produce different results on different
platforms.

Merging "./Inputs" and "source-interleave-x86_64.c" will use different
separators in POSIX and Windows.

Dedicated tests are needed for dealing with removing trailing separators
for POSIX (consider only '/') and Windows (consider '/' and '\').

Fixes D85024.
Fixes PR46368.

Diff Detail

Event Timeline

tinti created this revision.Jan 27 2021, 2:14 AM
tinti requested review of this revision.Jan 27 2021, 2:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 27 2021, 2:14 AM
tinti edited the summary of this revision. (Show Details)Jan 27 2021, 2:14 AM

Tested on Windows and Linux.

jhenderson added inline comments.Jan 27 2021, 2:19 AM
llvm/test/tools/llvm-objdump/X86/source-interleave-prefix-windows.test
1

Could you expand this comment to explain what the platform-specific behaviour is, please? (I'm guessing it's related to the trailing separator comment below).

11

Nit: no new line at EOF.

llvm/test/tools/llvm-objdump/X86/source-interleave-prefix.test
45

Nit: no new line at EOF.

tinti added inline comments.Jan 27 2021, 4:12 AM
llvm/test/tools/llvm-objdump/X86/source-interleave-prefix-windows.test
1

I think I can and you are right.

Take a look at this file llvm/test/tools/llvm-objdump/X86/Inputs/source-interleave.ll

...

!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 4.0.0", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, globals: !3)
!1 = !DIFile(filename: "source-interleave-x86_64.c", directory: "SRC_COMPDIR")
!2 = !{}

...

There is a directory entry and a file entry. SRC_COMPDIR will be replaced.

Merging "./Inputs" and "source-interleave-x86_64.c" will produce different results on platforms with different default separators.

Should I mention it on the code and/or commit message? Should I find where they are being merged (llc or llvm-objdump)?

jhenderson added inline comments.Jan 27 2021, 4:22 AM
llvm/test/tools/llvm-objdump/X86/source-interleave-prefix-windows.test
1

I'm mostly interested in a comment in the test that explains why there needs to be a dedicated Windows test and why it can't be part of the general test. It can be high-level, but needs to briefly say why just doing a regex to capture slashes in either direction is insufficient.

tinti updated this revision to Diff 319534.Jan 27 2021, 5:14 AM
tinti edited the summary of this revision. (Show Details)

Reword.
Explain behavior for adding {{[/\\]}}
Fix missing EOFs.

LGTM, with a couple of nits. Please leave it for about 24 hours to give others a chance to comment, should they wish.

llvm/test/tools/llvm-objdump/X86/source-interleave-prefix-non-windows.test
4 ↗(On Diff #319534)

system-windows is just a lit tag, and isn't the name of anything.

llvm/test/tools/llvm-objdump/X86/source-interleave-prefix-windows.test
5
tinti updated this revision to Diff 319540.Jan 27 2021, 5:44 AM

Reword as suggested.

tinti marked 6 inline comments as done.Jan 29 2021, 7:08 AM

Hi @jhenderson

Can you commit?

I'm about to sign off for the weekend, so if someone else can do it that would be good. Otherwise, I'll try to do this on Monday. Alternatively, you could always request commit access yourself?

jhenderson accepted this revision.Feb 1 2021, 2:36 AM

Forgot to click "Accept Revision". @tinti, have you asked for commit access?

This revision is now accepted and ready to land.Feb 1 2021, 2:36 AM
tinti added a comment.Feb 1 2021, 5:34 AM

Forgot to click "Accept Revision". @tinti, have you asked for commit access?

Yes I did. Thanks for the review.

This revision was automatically updated to reflect the committed changes.