Details
Diff Detail
- Build Status
Buildable 11265 Build 11265: arc lint + arc unit
Event Timeline
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.
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.
lld/test/COFF/linkrepro-pdb.test | ||
---|---|---|
2 | I don't believe this test needs requires shell does it? That will basically prevent it from running on Windows. |
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 | ||
---|---|---|
5–6 | Should these output files be going somewhere prefixed with %t? |
In case it wasn't clear, the idea behind tar tf was to run a FileCheck to verify the contents of the tar file.
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 | ||
---|---|---|
5–6 | 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.
I don't believe this test needs requires shell does it? That will basically prevent it from running on Windows.