This is an archive of the discontinued LLVM Phabricator instance.

gn build: Windows: use a more standard format for PDB filenames
ClosedPublic

Authored by dmajor on Feb 1 2019, 12:35 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

dmajor created this revision.Feb 1 2019, 12:35 PM
dmajor marked an inline comment as done.
dmajor added inline comments.
llvm/utils/gn/build/toolchain/BUILD.gn
249 ↗(On Diff #184815)

Does pdbfile need to be included in any of the above output(s) lines? (I don't know what they do.)

thakis accepted this revision.Feb 1 2019, 5:23 PM

Do you care much about this? The naming here might be somewhat unconventional, but it's more flexible (consider the completely hypothetical case of a build having both "chrome.exe" and "chorme.dll" – if you call the pdb for both "chrome.dll" they'll get in each other's way, but with this scheme it works fine) and ever so slightly less code. So I weakly prefer what we currently have, but I almost never do debug builds so I don't care all that much. I'll accept this so that you can land it if you want, but I'd weakly prefer if you didn't :-)

llvm/utils/gn/build/toolchain/BUILD.gn
249 ↗(On Diff #184815)

see llvm/utils/gn/gn.py help tool, section "outputs" :-P

Ninja knows that the link rule produces the outputs listed here if depend_output isn't set. Since depend_output _is_ set, I'm not sure what's in outputs matters all that much. If depend_output _wasn't_ set and pdbfile was in this list, then a build step that lists the pdb file as an input without depending on the executable() or shared_library() target that produces it would still correctly run the link to produce the pdb before running that step. But such a step should have a dep on the executable() / shared_library() for cleanliness anyhow, and the pdb is only written in debug builds, which we don't easily know if that's the case here, so I just omitted it.

This revision is now accepted and ready to land.Feb 1 2019, 5:23 PM
dmajor added a comment.Feb 4 2019, 8:19 AM

Do you care much about this? The naming here might be somewhat unconventional, but it's more flexible (consider the completely hypothetical case of a build having both "chrome.exe" and "chorme.dll" – if you call the pdb for both "chrome.dll" they'll get in each other's way, but with this scheme it works fine) and ever so slightly less code. So I weakly prefer what we currently have, but I almost never do debug builds so I don't care all that much. I'll accept this so that you can land it if you want, but I'd weakly prefer if you didn't :-)

I care about it mildly (a hair more than weakly :-P).

For better or worse this is the convention on Windows, including LLVM's cmake build and Microsoft's own published symbols, so this file doesn't seem like the right place to push for a change. If the cmake build changes its tune, I'm fine with you reverting this. :-)

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 4 2019, 1:20 PM