This is an archive of the discontinued LLVM Phabricator instance.

[llvm-ar][libObject] Fix relative paths when nesting thin archives.
ClosedPublic

Authored by rupprecht on Feb 6 2019, 1:11 PM.

Details

Summary

When adding one thin archive to another, we currently chop off the relative path to the flattened members. For instance, when adding foo/child.a (which contains x.txt) to parent.a, when flattening it we should add it as foo/x.txt (which exists) instead of x.txt (which does not exist).

As a note, this also undoes the IsNew parameter of handling relative paths in r288280. The unit test there still passes.

This was reported as part of testing the kernel build with llvm-ar: https://patchwork.kernel.org/patch/10767545/ (see the second point).

Diff Detail

Repository
rL LLVM

Event Timeline

rupprecht created this revision.Feb 6 2019, 1:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2019, 1:11 PM
ruiu added inline comments.Feb 6 2019, 1:21 PM
llvm/lib/Object/ArchiveWriter.cpp
238 ↗(On Diff #185624)

So, M.MemberName is relative to ArcName if it is thin, and it is used as-is if not thin. It doesn't look like we really have to distinguish two here. Could you call computeRelativePath early in llvm-ar and set its result to MemberName? Then I think you can remove special handling of thin archive here.

llvm/test/tools/llvm-ar/flatten-thin-archive-directories.test
15 ↗(On Diff #185624)

Are you going to fix this bug as well?

rupprecht marked 2 inline comments as done.Feb 6 2019, 1:33 PM
rupprecht added inline comments.
llvm/lib/Object/ArchiveWriter.cpp
238 ↗(On Diff #185624)

Hmm, good point, that would be a lot simpler. I'll try it.

llvm/test/tools/llvm-ar/flatten-thin-archive-directories.test
15 ↗(On Diff #185624)

I'll do it separately -- see D57845

rupprecht updated this revision to Diff 185644.Feb 6 2019, 2:25 PM
rupprecht marked an inline comment as done.
  • Move computeRelativePath special case from libObject to llvm-ar
  • Remove obsolete test fixme
rupprecht marked 3 inline comments as done.Feb 6 2019, 2:27 PM
rupprecht added inline comments.
llvm/lib/Object/ArchiveWriter.cpp
238 ↗(On Diff #185624)

That seems to have worked, thanks for the suggestion!

ruiu accepted this revision.Feb 6 2019, 2:39 PM

LGTM

llvm/lib/Object/ArchiveWriter.cpp
238 ↗(On Diff #185624)

This function is now too trivial to be a function. I'd remove this function by replacing a call site with OS << M.MemberName << "/\n";.

llvm/tools/llvm-ar/llvm-ar.cpp
618 ↗(On Diff #185644)

Remove the redundant assignment.

This revision is now accepted and ready to land.Feb 6 2019, 2:39 PM
rupprecht updated this revision to Diff 185650.Feb 6 2019, 3:02 PM
rupprecht marked 2 inline comments as done.
  • Fix related test to always delete the archive first
  • Inline addToStringTable
  • Remove redundant assignment
rupprecht marked an inline comment as done.Feb 6 2019, 3:02 PM
This revision was automatically updated to reflect the committed changes.
hans added a subscriber: hans.Feb 8 2019, 2:17 AM

I reverted this in r353507 as it broke the Chromium build; see https://crbug.com/930058

I reverted this in r353507 as it broke the Chromium build; see https://crbug.com/930058

Sorry for the breakage! Thanks for reverting this.

Do you have repro instructions? I'd like to reland this soon.

rupprecht reopened this revision.Feb 8 2019, 5:13 PM
This revision is now accepted and ready to land.Feb 8 2019, 5:13 PM
rupprecht updated this revision to Diff 186080.Feb 8 2019, 5:14 PM

Add regression test for chromium linking issue, and update LibDriver call site to correctly compute relative paths.

Reopened + updated patch with LibDriver fixes. I believe this should fix the chromium build, but haven't verified it. The new test case (llvm/test/tools/llvm-lib/thin-relative.test) should capture the issue, and was failing with the old patch but is now fixed.

rupprecht added a subscriber: tpimh.Feb 8 2019, 5:20 PM
hans added a comment.Feb 12 2019, 12:28 AM

Reopened + updated patch with LibDriver fixes. I believe this should fix the chromium build, but haven't verified it. The new test case (llvm/test/tools/llvm-lib/thin-relative.test) should capture the issue, and was failing with the old patch but is now fixed.

Thanks! I'm not the right person to review this, but I can keep an eye on our buildbots when it lands.

ruiu accepted this revision.Feb 12 2019, 2:16 PM

LGTM

void added a subscriber: void.Feb 13 2019, 2:25 PM
This revision was automatically updated to reflect the committed changes.