This is an archive of the discontinued LLVM Phabricator instance.

[llvm-ar] Make llvm-lib behave more like the MSVC archiver
ClosedPublic

Authored by rnk on May 25 2017, 4:13 PM.

Details

Summary

Use the filepath used to open the archive member as the archive member
name instead of the file basename. This path might be absolute or
relative. This is important because the archive member name will show
up in the PDB, and we want our PDBs to look as much like MSVC's as
possible.

This also helps avoid an issue in our PDB module descriptor writing
code, which assumes that all module names are unique. Relative paths
still aren't guaranteed to be unique, but they're much better than
basenames, which definitely aren't unique.

Event Timeline

rnk created this revision.May 25 2017, 4:13 PM
ruiu added inline comments.May 25 2017, 4:21 PM
llvm/lib/Object/ArchiveWriter.cpp
187

If it fits in 16 byte buffer, we can write there even if a path contains a path separator, no?

244

Debug output?

zturner edited edge metadata.May 25 2017, 4:26 PM

Why do we even need WriteObjPaths? Can't we just standardize on true for writing, but still support false for reading?

llvm/lib/Object/ArchiveWriter.cpp
206–207

For some reason I thought we had an llvm function to do this, essentially mimicing the behavior of os.path.commonprefix in python. I guess I was wrong though.

220–221

Is this what we want? If we're producing an archive to be used on Windows, it seems like we would want it to be as close to MSVC's behavior as possible, which is going to be to use backslashes (on Windows anyway). It seems like this could confuse MSVC's linker if we feed it archives that have forward slashes.

224

I don't think this is entirely what you want. This means more than just "use forward slashes". It means "use the entire posix path grammar". Valid filename characters, drive letter parsing, etc all depend on this. I suspect we'll run into subtle changes with this. If you really just want to use slashes, use the native host path style, and at the end write replace(str, '\\', '/');

rnk marked an inline comment as done.May 25 2017, 4:45 PM
rnk added inline comments.
llvm/lib/Object/ArchiveWriter.cpp
187

I don't think so. '/' seems to be a special character inside the short name.

rnk updated this revision to Diff 100338.May 25 2017, 4:45 PM
  • remove errs prints
rnk added inline comments.May 25 2017, 4:52 PM
llvm/lib/Object/ArchiveWriter.cpp
224

That's awful. I'll go ahead and revert my changes here. I just wanted to get rid of #ifdef LLVM_ON_WIN32. :(

rnk updated this revision to Diff 100341.May 25 2017, 4:54 PM
  • revert path append changes
rnk updated this revision to Diff 100460.May 26 2017, 1:18 PM
  • Compute all member names up front
rnk updated this revision to Diff 100468.May 26 2017, 1:39 PM
  • Preserve old member names when updating archives
pcc added a subscriber: pcc.May 26 2017, 1:51 PM
pcc added inline comments.
llvm/include/llvm/Object/ArchiveWriter.h
41

Would it be simpler to store the member name in NewArchiveMember and pass WriteObjPaths to NewArchiveMember::getFile()?

llvm/lib/Object/ArchiveWriter.cpp
194

Looks like WriteObjPaths is unused in this function.

rnk added inline comments.May 26 2017, 2:12 PM
llvm/include/llvm/Object/ArchiveWriter.h
41

We'd still need to re-relativize that path for thin archives during writeArchive, though. I like the way the name computation logic is all in the same place with the current patch.

llvm/lib/Object/ArchiveWriter.cpp
194

True, we can get rid of it now.

rnk updated this revision to Diff 100493.May 26 2017, 4:24 PM
  • remove dead bool
ruiu edited edge metadata.Jun 12 2017, 10:14 AM

I wonder how many uses of this class in LLVM. I'm asking because it feels more cleaner to me if you remove WriteObjPaths parameter from writeArchive and strip directories if it's needed on the client side.

llvm/lib/Object/ArchiveWriter.cpp
290

This could be

(Twine(/) + Twine(StringMapIndex)).str();

?

rnk updated this revision to Diff 102211.Jun 12 2017, 11:36 AM

Rewrite the patch to store the name in NewArchiveMember

rnk added a comment.Jun 12 2017, 12:32 PM

I rewrote the patch to store the name in the NewArchiveMember. I think initially I avoided this approach because it duplicates the member name between the MemoryBuffer and the NewArchiveMember, and it still isn't the final name for thin archives. I kind of liked centralizing the member name calculation logic in writeArchive. However, looking at it now, this definitely feels cleaner. :)

ruiu accepted this revision.Jun 12 2017, 12:38 PM

LGTM

This looks much better!

This revision is now accepted and ready to land.Jun 12 2017, 12:38 PM
This revision was automatically updated to reflect the committed changes.