-
Notifications
You must be signed in to change notification settings - Fork 247
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
implement -gsplit-dwarf (dwarf fission) support #144
Conversation
- introduces a protocol bump to 35. the output directory given on the command line and whether dwarf fission (-gsplit-dwarf) is enabled are now passed in CompileFileMsg; CompileResultMsg now contains whether there is a dwo file to receive - These changes should be backward compatible in both directions (newer client and older daemons as well as newer daemons with older clients) - gcc hard codes the path to the dwo file based on the given output file path, which requires some clever path manipulation in the remote environment - the path manipulation is also sufficient for clang to embed the correct information
- requires comparing portions of readelf instead of the full file since slight differences between remote and local make the former impossible
That's an impressive work and I can't see anything wrong with it. I can merge it, but I would prefer giving over maintainership of the repo to you. You seem to understand the code well enough to be the maintainer of it :) |
Oh wow. Let me consider taking maintainership a little bit first but go ahead and merge for now if you feel comfortable with it. Thanks for taking a look! |
107bcaa
to
9c254b7
Compare
…ctness and safety
9c254b7
to
972eeab
Compare
I just pushed on more commit as I forgot to include the size of the dwo file in the job statistics. Let me know if you'd like me to address anything else before merging. |
@@ -176,7 +176,7 @@ static int create_native(char **args) | |||
is_clang = true; | |||
// Args[0] may be a compiler or the first extra file. | |||
if (args[0] && ((!strcmp(args[0], "clang") && (is_clang = true)) | |||
|| (!strcmp(args[0], "gcc") && (is_clang = false)))) { | |||
|| (!strcmp(args[0], "gcc") && !(is_clang = false)))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is merged you may close #124 since it contains essentially this fix. Your way of fixing it is terser and probably readable enough for the audience anyway.
From a cursory reading of the changeset, it looks like this could be extended / generalised to add support for precompiled headers. Currently icecream just decides to compile any files that need PCH on the local machine rather than sending across the PCH files similarly to this. |
A discussion about the possibility of
-gsplit-dwarf
support feature request is in #86. For more details about the functionality, please see https://gcc.gnu.org/wiki/DebugFission.This pull request adds full distributed compilation support when using the
-gsplit-dwarf
flag for both gcc and clang.There is an oddity with how the compilers embed the
.dwo
file path information into the object file, and is based on the specified path to the output file on the invoked command line. Some utility methods (shamelessly adapted from various Stack Overflow discussions) aids path manipulation withoutboost::filesystem
. These path manipulation techniques allow the daemon to recreate the directory structure present on the client, thus tricking the compiler into embedding.dwo
path information that will match what the client's machine expects.Tests have been added, though I had to resort to comparing
readelf
output whilst ignoring some debugging specific information since certain tags (DW_AT_GNU_dwo_id
, for instance) are different on each run.DW_AT_GNU_dwo_name
andDW_AT_comp_dir
will be slightly different based on whether it was compiled remotely or locally, as well, since the path manipulation trick is not necessary for local builds.