keep order of remote arguments, especially -gsplit-dwarf (icecc#435)

This is based on pull requests icecc#425. Both gcc and clang have their peculiarities when handling -gsplit-dwarf depending on the exact order of this and other -g arguments. So keep the order the same and do not handle any of those arguments specially. This technically changes the protocol, but let's keep it as part of the recent version 41 change.
dmoody256 · Jun 29, 2019 · 8801771 · 8801771
1 parent 2eed5e3
commit 8801771
Show file tree

Hide file tree

Showing 8 changed files with 66 additions and 38 deletions.
diff --git a/client/arg.cpp b/client/arg.cpp
@@ -293,7 +293,6 @@ bool analyse_argv(const char * const *argv, CompileJob &job, bool icerun, list<s
 {
     ArgumentsList args;
     string ofile;
-    string dwofile;
 
 #if CLIENT_DEBUG > 1
     trace() << "scanning arguments" << endl;
@@ -445,6 +444,7 @@ bool analyse_argv(const char * const *argv, CompileJob &job, bool icerun, list<s
                 always_local = true;
                 args.append(a, Arg_Local);
             } else if (!strcmp(a, "-gsplit-dwarf")) {
+                args.append(a, Arg_Rest);
                 seen_split_dwarf = true;
             } else if (str_equal(a, "-x")) {
                 args.append(a, Arg_Rest);

diff --git a/client/local.cpp b/client/local.cpp
@@ -251,10 +251,6 @@ int build_local(CompileJob &job, MsgChannel *local_daemon, struct rusage *used)
     arguments.push_back(compiler_name);
     appendList(arguments, job.allFlags());
 
-    if (job.dwarfFissionEnabled()) {
-        arguments.push_back("-gsplit-dwarf");
-    }
-
     if (!job.inputFile().empty()) {
         arguments.push_back(job.inputFile());
     }

diff --git a/daemon/serve.cpp b/daemon/serve.cpp
@@ -286,18 +286,19 @@ int handle_connection(const string &basedir, CompileJob *job,
             }
         }
 
-        if (!client->send_msg(rmsg)) {
-            log_info() << "write of result failed" << endl;
-            throw myexception(EXIT_DISTCC_FAILED);
-        }
-
         struct stat st;
-
-        if (!stat(obj_file.c_str(), &st)) {
+        if (stat(obj_file.c_str(), &st) == 0) {
             job_stat[JobStatistics::out_uncompressed] += st.st_size;
         }
-        if (!stat(dwo_file.c_str(), &st)) {
+        if (stat(dwo_file.c_str(), &st) == 0) {
             job_stat[JobStatistics::out_uncompressed] += st.st_size;
+            rmsg.have_dwo_file = true;
+        } else
+            rmsg.have_dwo_file = false;
+
+        if (!client->send_msg(rmsg)) {
+            log_info() << "write of result failed" << endl;
+            throw myexception(EXIT_DISTCC_FAILED);
         }
 
         /* wake up parent and tell him that compile finished */

diff --git a/daemon/workit.cpp b/daemon/workit.cpp
@@ -103,10 +103,9 @@ int work_it(CompileJob &j, unsigned int job_stat[], MsgChannel *client, CompileR
     rmsg.out.erase(rmsg.out.begin(), rmsg.out.end());
     rmsg.out.erase(rmsg.out.begin(), rmsg.out.end());
 
-    std::list<string> list = j.remoteFlags();
-    appendList(list, j.restFlags());
+    std::list<string> list = j.nonLocalFlags();
 
-    if (j.dwarfFissionEnabled()) {
+    if (!IS_PROTOCOL_41(client) && j.dwarfFissionEnabled()) {
         list.push_back("-gsplit-dwarf");
     }
 
@@ -716,7 +715,6 @@ int work_it(CompileJob &j, unsigned int job_stat[], MsgChannel *client, CompileR
                     struct timeval endtv;
                     gettimeofday(&endtv, 0);
                     rmsg.status = shell_exit_status(status);
-                    rmsg.have_dwo_file = j.dwarfFissionEnabled();
                     job_stat[JobStatistics::exit_code] = shell_exit_status(status);
                     job_stat[JobStatistics::real_msec] = ((endtv.tv_sec - starttv.tv_sec) * 1000)
                                                          + ((long(endtv.tv_usec) - long(starttv.tv_usec)) / 1000);

diff --git a/services/comm.cpp b/services/comm.cpp
@@ -2025,24 +2025,29 @@ void CompileFileMsg::fill_from_channel(MsgChannel *c)
 {
     Msg::fill_from_channel(c);
     uint32_t id, lang;
-    list<string> _l1, _l2;
     string version;
     *c >> lang;
     *c >> id;
-    *c >> _l1;
-    *c >> _l2;
+    ArgumentsList l;
+    if( IS_PROTOCOL_41(c)) {
+        list<string> largs;
+        *c >> largs;
+        // Whe compiling remotely, we no longer care about the Arg_Remote vs Arg_Rest
+        // difference, so treat them all as Arg_Remote.
+        for (list<string>::const_iterator it = largs.begin(); it != largs.end(); ++it)
+            l.append(*it, Arg_Remote);
+    } else {
+        list<string> _l1, _l2;
+        for (list<string>::const_iterator it = _l1.begin(); it != _l1.end(); ++it)
+            l.append(*it, Arg_Remote);
+        for (list<string>::const_iterator it = _l2.begin(); it != _l2.end(); ++it)
+            l.append(*it, Arg_Rest);
+        *c >> _l1;
+        *c >> _l2;
+    }
     *c >> version;
     job->setLanguage((CompileJob::Language) lang);
     job->setJobID(id);
-    ArgumentsList l;
-
-    for (list<string>::const_iterator it = _l1.begin(); it != _l1.end(); ++it) {
-        l.append(*it, Arg_Remote);
-    }
-
-    for (list<string>::const_iterator it = _l2.begin(); it != _l2.end(); ++it) {
-        l.append(*it, Arg_Rest);
-    }
 
     job->setFlags(l);
     job->setEnvironmentVersion(version);
@@ -2080,20 +2085,27 @@ void CompileFileMsg::send_to_channel(MsgChannel *c) const
     *c << (uint32_t) job->language();
     *c << job->jobID();
 
-    if (IS_PROTOCOL_30(c)) {
-        *c << job->remoteFlags();
+    if (IS_PROTOCOL_41(c)) {
+        // By the time we're compiling, the args are all Arg_Remote or Arg_Rest and
+        // we no longer care about the differences, but we may care about the ordering.
+        // So keep them all in one list.
+        *c << job->nonLocalFlags();
     } else {
-        if (job->compilerName().find("clang") != string::npos) {
-            // Hack for compilerwrapper.
-            std::list<std::string> flags = job->remoteFlags();
-            flags.push_front("clang");
-            *c << flags;
-        } else {
+        if (IS_PROTOCOL_30(c)) {
             *c << job->remoteFlags();
+        } else {
+            if (job->compilerName().find("clang") != string::npos) {
+                // Hack for compilerwrapper.
+                std::list<std::string> flags = job->remoteFlags();
+                flags.push_front("clang");
+                *c << flags;
+            } else {
+                *c << job->remoteFlags();
+            }
         }
+        *c << job->restFlags();
     }
 
-    *c << job->restFlags();
     *c << job->environmentVersion();
     *c << job->targetPlatform();
 

diff --git a/services/job.cpp b/services/job.cpp
@@ -56,6 +56,19 @@ list<string> CompileJob::restFlags() const
     return flags(Arg_Rest);
 }
 
+list<string> CompileJob::nonLocalFlags() const
+{
+    list<string> args;
+
+    for (ArgumentsList::const_iterator it = m_flags.begin(); it != m_flags.end(); ++it) {
+        if (it->second != Arg_Local) {
+            args.push_back(it->first);
+        }
+    }
+
+    return args;
+}
+
 list<string> CompileJob::allFlags() const
 {
     list<string> args;

diff --git a/services/job.h b/services/job.h
@@ -122,6 +122,7 @@ class CompileJob
     std::list<std::string> localFlags() const;
     std::list<std::string> remoteFlags() const;
     std::list<std::string> restFlags() const;
+    std::list<std::string> nonLocalFlags() const;
     std::list<std::string> allFlags() const;
 
     void setInputFile(const std::string &file)
@@ -144,6 +145,7 @@ class CompileJob
         return m_output_file;
     }
 
+    // Since protocol 41 this is just a shortcut saying that allFlags() contains "-gsplit-dwarf".
     void setDwarfFissionEnabled(bool flag)
     {
         m_dwarf_fission = flag;

diff --git a/tests/test.sh b/tests/test.sh
@@ -1209,6 +1209,10 @@ debug_test()
         stop_ice 0
         abort_tests
     fi
+    # Binaries without debug infos use hex addresses for some symbols, which may differ between runs
+    # or builds, but is technically harmless. So remove symbol and stack addresses and let the readelf check handle that.
+    sed -i -e 's/=0x[0-9a-fA-F]*//g' "$testdir"/debug-output-remote.txt
+    sed -i -e 's/=0x[0-9a-fA-F]*//g' "$testdir"/debug-output-local.txt
     if ! diff "$testdir"/debug-output-local.txt "$testdir"/debug-output-remote.txt >/dev/null; then
         echo Gdb output different.
         echo =====================
@@ -1842,9 +1846,11 @@ if command -v gdb >/dev/null; then
     if command -v readelf >/dev/null; then
         debug_test "$TESTCXX" "-c -g debug.cpp" "Temporary breakpoint 1, main () at debug.cpp:8"
         debug_test "$TESTCXX" "-c -g $(pwd)/debug/debug2.cpp" "Temporary breakpoint 1, main () at .*debug/debug2.cpp:8"
+        debug_test "$TESTCXX" "-c -g0 debug.cpp" "Temporary breakpoint 1, 0x"
         if test -z "$debug_fission_disabled"; then
             debug_test "$TESTCXX" "-c -g debug.cpp -gsplit-dwarf" "Temporary breakpoint 1, main () at debug.cpp:8"
             debug_test "$TESTCXX" "-c -g $(pwd)/debug/debug2.cpp -gsplit-dwarf" "Temporary breakpoint 1, main () at .*debug/debug2.cpp:8"
+            debug_test "$TESTCXX" "-c debug.cpp -gsplit-dwarf -g0" "Temporary breakpoint 1, 0x"
         fi
     fi
 else