Page MenuHomePhabricator

[llvm-objdump] Implement --prefix-strip option
ClosedPublic

Authored by tinti on Feb 14 2021, 6:39 PM.

Details

Summary

The option --prefix-strip is only used when --prefix is not empty.
It removes N initial directories from absolute paths before adding the
prefix.

This matches GNU's objdump behavior.

Diff Detail

Event Timeline

tinti created this revision.Feb 14 2021, 6:39 PM
tinti requested review of this revision.Feb 14 2021, 6:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 14 2021, 6:39 PM
tinti updated this revision to Diff 323658.Feb 14 2021, 6:46 PM

Fix patch does not apply.

jhenderson added inline comments.Feb 17 2021, 1:30 AM
llvm/test/tools/llvm-objdump/X86/source-interleave-prefix.test
45
51

Looks like there's one too many blank lines at the end of this file?

Also, I think you should have a few additional test cases:

  1. A --prefix-strip value of 0.
  2. A --prefix-strip value equal to the number of directory components.
  3. A --prefix-strip value greater than the number of components.
llvm/tools/llvm-objdump/llvm-objdump.cpp
361

Are you sure you want int? That implies this can take a negative number. Likely you want something like uint32_t...

1042

Perhaps worth an assert that shows that LineInfo.FileName is not empty.

1046–1052

Have you looked at using the path component iterator functionality provided by Path.h to do this instead? It seems like that would be a preferable solution to your hand-rolled version here.

3000–3001

If you don't use int, then this will be sorted by the command-line library itself.

tinti marked 5 inline comments as done.Feb 18 2021, 6:57 AM
tinti added inline comments.
llvm/test/tools/llvm-objdump/X86/source-interleave-prefix.test
51

I agree.

llvm/tools/llvm-objdump/llvm-objdump.cpp
361

I agree.

1042

Yes. I will place it just after is_absolute_gnu.

1046–1052

Yes I have. But it skips extra separators which is not what GNU does.

https://github.com/llvm/llvm-project/blob/004a264f8c923922ecd34255bcb10f4adaa27ac5/llvm/lib/Support/Path.cpp#L267

To make '/foo//bar/baz' become '/baz' --prefix-strip must be 3.

I will add a test for it too.

tinti marked 4 inline comments as done.Feb 18 2021, 4:52 PM
tinti updated this revision to Diff 324815.Feb 18 2021, 5:04 PM

Add range tests.
Add extra separator test.
Add assert.
Use uint32_t instead of int.
Comment about not using Path.h iterator.

tinti marked an inline comment as done.Feb 18 2021, 5:05 PM
jhenderson added inline comments.Feb 26 2021, 12:34 AM
llvm/docs/CommandGuide/llvm-objdump.rst
178

Same in the man page.

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

I think it would be worth making this and the following cases positive cases (i.e. where the behaviour results in a valid source interleave), if possible. Also, I think adding to this comment what the path before and after the --prefix-strip would be, as it makes it easier to envision what is going on.

72

Perhaps also worth testing a negative value to show that llvm-objdump reports an error in this case.

llvm/tools/llvm-objdump/llvm-objdump.cpp
364
1045–1054

I think you can avoid converting to a C-string here, by doing something like the following, if I'm not mistaken:

auto StrippedNameStart = LineInfo.FileName.begin();
for (auto Pos = LineInfo.FileName.begin() + 1, End = LineInfo.FileName.end(); Pos != End && Level < PrefixStrip; ++Pos) {
  if (sys::path::is_separator(*Pos)) {
    StrippedNameStart = Pos;
    ++Level;
  }
}
LineInfo.FileName = std::string(Post, LineInfo.FileName.end());
tinti updated this revision to Diff 326650.Feb 26 2021, 3:53 AM

Reword documentation.
Use good result test instead of bad result.
Add invalid value test.
Convert C code to C++.

tinti marked 5 inline comments as done.Feb 26 2021, 3:54 AM

Sorry for the delay. I've been off sick for much of the week, and am still catching up on things.

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

Another one that has occurred to me: testing a non-numeric value, e.g. "--prefix-strip=foo" to show an error is hit. Not 100% certain if it's necessary, but it ensures we have "invalid case" handling in place, in case negative numbers are ever accepted for some reason.

tinti updated this revision to Diff 328751.Mar 6 2021, 2:27 AM
tinti edited the summary of this revision. (Show Details)

Add invalid text test.

tinti marked an inline comment as done.Mar 6 2021, 2:28 AM

Sorry for the delay. I've been off sick for much of the week, and am still catching up on things.

I hope you are well. Be safe.

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

I agree.

This revision is now accepted and ready to land.Mar 7 2021, 11:59 PM
This revision was landed with ongoing or failed builds.Mar 24 2021, 6:27 AM
This revision was automatically updated to reflect the committed changes.
tinti marked an inline comment as done.