This is an archive of the discontinued LLVM Phabricator instance.

COFF: Add type server pdb files to linkrepro tar file.
ClosedPublic

Authored by pcc on Oct 16 2017, 2:29 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

pcc created this revision.Oct 16 2017, 2:29 PM
pcc updated this revision to Diff 119217.Oct 16 2017, 2:31 PM

Add missing REQUIRES line

zturner edited edge metadata.Oct 16 2017, 2:43 PM

It's possible I'm missing something here, but I don't see anything specific to linkrepro or TAR files in any of this patch. This leads me to believe /LINKREPRO is doing a full PDB link, including merging types and symbols from all object files. resolving type servers and merging those in as well.

If I understand this feature correctly, /LINKREPRO isn't actually supposed to link anything. It just copies the necessary files to a TAR file and creates a response file so that that response file, combined with the TAR file can then be linked on a different machine.

If my understanding is wrong feel free to correct me, but if it's right, then this seems overly heavyweight. It looks like this is doing a full PDB link but injecting some sort of Writer class that instead of writing to an output executable + PDB file, writes to a TAR file.

When we discussed this on IRC, I thought the solution was just going to find the type server PDB and add it to the tar file, but not actually link or do anything expensive.

pcc added a comment.Oct 16 2017, 3:10 PM

It's possible I'm missing something here, but I don't see anything specific to linkrepro or TAR files in any of this patch.

The important part of this patch is the call to takeBuffer on line 229 of PDB.cpp. That will cause the argument to be added to the linkrepro if we are creating one. Admittedly it could be a little clearer what that function does, we may want to rename it now that we're starting to use it outside of the driver.

This leads me to believe /LINKREPRO is doing a full PDB link, including merging types and symbols from all object files. resolving type servers and merging those in as well.

If I understand this feature correctly, /LINKREPRO isn't actually supposed to link anything. It just copies the necessary files to a TAR file and creates a response file so that that response file, combined with the TAR file can then be linked on a different machine.

That's not accurate. /linkrepro in LLD is implemented by adding files to the linkrepro tar file as a side effect of opening files during a full link (note that link.exe's implementation of linkrepro also does a full link). I suppose that it may be possible in principle to implement /linkrepro so that it just creates the tar file, but it seems simplest to do things this way to reduce the possibility of divergence between what linkrepro does and what regular links do.

If my understanding is wrong feel free to correct me, but if it's right, then this seems overly heavyweight. It looks like this is doing a full PDB link but injecting some sort of Writer class that instead of writing to an output executable + PDB file, writes to a TAR file.

No, the output files are not written to the tar file, only the inputs.

zturner added inline comments.Oct 17 2017, 2:51 PM
lld/test/COFF/linkrepro-pdb.test
1 ↗(On Diff #119217)

I don't believe this test needs requires shell does it? That will basically prevent it from running on Windows.

pcc added inline comments.Oct 17 2017, 2:57 PM
lld/test/COFF/linkrepro-pdb.test
1 ↗(On Diff #119217)

I think the linkrepro tests do in fact require not-Windows, perhaps because of path length limitations. I think Rui did try to enable them on Windows at one point, but that change had to be reverted (see rL291527).

ruiu edited edge metadata.Oct 17 2017, 3:06 PM

Yeah, some tests failed on some bots due to the maximum path limitation on Windows. /linkrepro actually works just fine, but in order to verify its contents, we have to untar the produced file into a working directory. That sometimes fails because the length of the current working directory + pathnames in the tar could easily exceed 255 characters long on some bots.

In D38977#900140, @ruiu wrote:

Yeah, some tests failed on some bots due to the maximum path limitation on Windows. /linkrepro actually works just fine, but in order to verify its contents, we have to untar the produced file into a working directory. That sometimes fails because the length of the current working directory + pathnames in the tar could easily exceed 255 characters long on some bots.

Would it be sufficient to:

a) Use tar tf to list the contents
b) untar the file you're interested in like this:

RUN: tar xOf repro.tar %t/ts.pdb > %t/repro/ts.pdb
RUN: diff %t/ts.pdb %t/repro/ts.pdb
lld/test/COFF/linkrepro-pdb.test
4–5 ↗(On Diff #119217)

Should these output files be going somewhere prefixed with %t?

In D38977#900140, @ruiu wrote:

Yeah, some tests failed on some bots due to the maximum path limitation on Windows. /linkrepro actually works just fine, but in order to verify its contents, we have to untar the produced file into a working directory. That sometimes fails because the length of the current working directory + pathnames in the tar could easily exceed 255 characters long on some bots.

Would it be sufficient to:

a) Use tar tf to list the contents
b) untar the file you're interested in like this:

RUN: tar xOf repro.tar %t/ts.pdb > %t/repro/ts.pdb
RUN: diff %t/ts.pdb %t/repro/ts.pdb

In case it wasn't clear, the idea behind tar tf was to run a FileCheck to verify the contents of the tar file.

ruiu added a comment.Oct 17 2017, 3:26 PM

If not GNU but all tar commands support the 'O' option, then yes, that should suffice.

pcc added a comment.Oct 17 2017, 3:35 PM
In D38977#900155, @ruiu wrote:

If not GNU but all tar commands support the 'O' option, then yes, that should suffice.

Seems worth a try. Looks like gnuwin32 and Darwin tar also support it, and so does FreeBSD: https://github.com/freebsd/freebsd/blob/master/contrib/libarchive/tar/bsdtar.c#L528

lld/test/COFF/linkrepro-pdb.test
4–5 ↗(On Diff #119217)

We do cd %t above, so they'll end up in that directory.

OpenBSD is the only one I can find after a quick search whose tar doesn't support -O. or at the very least, the man page I found doesn't. On the other hand, if 1-2 platforms are going to have to be disabled no matter what because there is no way to write it for all platforms, and we can choose between "enabled on Windows, disabled on platform X" versus "enabled on platform X, disabled on Windows", I think we should prefer the former, since this is in the Windows linker.

ruiu added a comment.Oct 17 2017, 4:10 PM

Maybe you want to add gnutar predicate so that you can write REQUIRES: gnutar?

pcc updated this revision to Diff 119398.Oct 17 2017, 4:16 PM

Use "tar xOf" to try to avoid path length limitations on Windows

Depends on D39023

pcc updated this revision to Diff 119399.Oct 17 2017, 4:30 PM
  • Add a gnutar feature and start using it for the test
ruiu accepted this revision.Oct 20 2017, 12:00 PM

LGTM

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