From 364159d8d024a783db44cb0a5b81e94f87f10ef4 Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Tue, 11 Feb 2020 11:43:56 +0200 Subject: Defer unknown header failure to compiler diagnostics --- libbuild2/cc/compile-rule.cxx | 271 +++++++++++++++++++++++++++++------------- libbuild2/cc/compile-rule.hxx | 2 +- 2 files changed, 192 insertions(+), 81 deletions(-) diff --git a/libbuild2/cc/compile-rule.cxx b/libbuild2/cc/compile-rule.cxx index 6fac82a..26df353 100644 --- a/libbuild2/cc/compile-rule.cxx +++ b/libbuild2/cc/compile-rule.cxx @@ -166,6 +166,7 @@ namespace build2 unit_type type; preprocessed pp = preprocessed::none; + bool deferred_failure = false; // Failure deferred to compilation. bool symexport = false; // Target uses __symexport. bool touch = false; // Target needs to be touched. timestamp mt = timestamp_unknown; // Target timestamp. @@ -985,6 +986,11 @@ namespace build2 // Store the translation unit's checksum to detect ignorable changes // (whitespaces, comments, etc). // + // Note that we skip all of this if we have deferred a failure from + // the header extraction phase (none of the module information should + // be relevant). + // + if (!md.deferred_failure) { optional cs; if (string* l = dd.read ()) @@ -1968,13 +1974,25 @@ namespace build2 break; } - // If we couldn't enter this header as a target (as opposed to - // not finding a rule to update it), then our diagnostics won't - // really add anything to the compiler's. + // If we couldn't enter this header as a target or find a rule + // to update it, then it most likely means a misspelled header + // (rather than a broken generated header setup) and our + // diagnostics won't really add anything to the compiler's. So + // let's only print it at -V or higher. // if (ht == nullptr) { assert (!exists); // Sanity check. + + if (verb > 2) + { + diag_record dr; + dr << error << "header '" << f << "' not found"; + + if (verb < 4) + dr << info << "re-run with --verbose=4 for more information"; + } + throw failed (); } @@ -1987,9 +2005,11 @@ namespace build2 if (pts.empty () || pts.back () != ht) { optional ir (inject_header (a, t, - *ht, false /* cache */, - timestamp_unknown)); - assert (ir); // Not from cache. + *ht, timestamp_unknown, + verb > 2 /* fail */)); + if (!ir) + throw failed (); + updated = *ir; } else @@ -2063,8 +2083,8 @@ namespace build2 if (!skip) { optional ir (inject_header (a, t, - bt, false /* cache */, - timestamp_unknown)); + bt, timestamp_unknown, + true /* fail */)); assert (ir); // Not from cache. update = *ir || update; } @@ -2480,41 +2500,35 @@ namespace build2 } // Update and add to the list of prerequisite targets a header or header - // unit target. Depending on the cache flag, the target is assumed to - // either have come from the depdb cache or from the compiler run. + // unit target. // // Return the indication of whether it has changed or, if the passed // timestamp is not timestamp_unknown, is older than the target. If the - // header came from the cache and it no longer exists nor can be - // generated, then return nullopt. + // header does not exists nor can be generated (no rule), then issue + // diagnostics and fail if the fail argument is true and return nullopt + // otherwise. // // Note: this used to be a lambda inside extract_headers() so refer to the // body of that function for the overall picture. // optional compile_rule:: inject_header (action a, file& t, - const file& pt, bool cache, timestamp mt) const + const file& pt, timestamp mt, bool f /* fail */) const { tracer trace (x, "compile_rule::inject_header"); - // Match to a rule. - // - // If we are reading the cache, then it is possible the file has since - // been removed (think of a header in /usr/local/include that has been - // uninstalled and now we need to use one from /usr/include). This will - // lead to the match failure which we translate to a restart. - // - // And if this is not a cached entry, then we still use try_match() in - // order to issue consistent (with extract_headers() below) diagnostics - // (rather than the generic "not rule to update ..."). + // Even if failing we still use try_match() in order to issue consistent + // (with extract_headers() below) diagnostics (rather than the generic + // "not rule to update ..."). // if (!try_match (a, pt).first) { - if (cache) + if (!f) return nullopt; diag_record dr; - dr << fail << "header " << pt << " not found and cannot be generated"; + dr << fail << "header " << pt << " not found and no rule to " + << "generate it"; if (verb < 4) dr << info << "re-run with --verbose=4 for more information"; @@ -3249,27 +3263,46 @@ namespace build2 // Enter as a target, update, and add to the list of prerequisite // targets a header file. Depending on the cache flag, the file is // assumed to either have come from the depdb cache or from the compiler - // run. Return true if the extraction process should be restarted. + // run. Return true if the extraction process should be restarted and + // false otherwise. Return nullopt if the header is not found and cannot + // be generated, the diagnostics has been issued, but the failure has + // been deferred to the compiler run in order to get better diagnostics. // auto add = [a, &bs, &t, li, &pfx_map, &so_map, &dd, &skip_count, - this] (path hp, bool cache, timestamp mt) -> bool + this] (path hp, bool cache, timestamp mt) -> optional { + context& ctx (t.ctx); + + // We can only defer the failure if we will be running the compiler. + // Let's also only do it in the "keep going" mode. + // + bool df (!ctx.dry_run_option && ctx.keep_going); + const file* ht (enter_header (a, bs, t, li, move (hp), cache, pfx_map, so_map).first); if (ht == nullptr) { diag_record dr; - dr << fail << "header '" << hp - << "' not found and cannot be generated"; + dr << error << "header '" << hp << "' not found"; + + if (df) + dr << info << "failure deferred to compiler diagnostics"; if (verb < 4) dr << info << "re-run with --verbose=4 for more information"; + + if (df) return nullopt; else dr << endf; } - if (optional u = inject_header (a, t, *ht, cache, mt)) + // If we are reading the cache, then it is possible the file has since + // been removed (think of a header in /usr/local/include that has been + // uninstalled and now we need to use one from /usr/include). This + // will lead to the match failure which we translate to a restart. + // + if (optional u = inject_header (a, t, *ht, mt, false /* fail */)) { // Verify/add it to the dependency database. // @@ -3279,6 +3312,20 @@ namespace build2 skip_count++; return *u; } + else if (!cache) + { + diag_record dr; + dr << error << "header " << *ht << " not found and no rule to " + << "generate it"; + + if (df) + dr << info << "failure deferred to compiler diagnostics"; + + if (verb < 4) + dr << info << "re-run with --verbose=4 for more information"; + + if (df) return nullopt; else dr << endf; + } dd.write (); // Invalidate this line. return true; @@ -3290,25 +3337,32 @@ namespace build2 auto add_unit = [a, &bs, &t, li, &pfx_map, &so_map, &dd, &skip_count, &md, - this] (path hp, path bp, timestamp mt) -> bool + this] (path hp, path bp, timestamp mt) -> optional { + context& ctx (t.ctx); + bool df (!ctx.dry_run_option && ctx.keep_going); + const file* ht (enter_header (a, bs, t, li, move (hp), true /* cache */, pfx_map, so_map).first); if (ht == nullptr) { diag_record dr; - dr << fail << "header '" << hp - << "' not found and cannot be generated"; + dr << error << "header '" << hp << "' not found"; + + if (df) + dr << info << "failure deferred to compiler diagnostics"; if (verb < 4) dr << info << "re-run with --verbose=4 for more information"; + + if (df) return nullopt; else dr << endf; } // Again, looks like we have to update the header explicitly since // we want to restart rather than fail if it cannot be updated. // - if (inject_header (a, t, *ht, true /* cache */, mt)) + if (inject_header (a, t, *ht, mt, false /* fail */)) { const file& bt (make_header_sidebuild (a, bs, li, *ht)); @@ -3316,8 +3370,7 @@ namespace build2 // the header, we should be able to build its BMI. In other words, a // restart is not going to change anything. // - optional u (inject_header (a, t, - bt, false /* cache */, mt)); + optional u (inject_header (a, t, bt, mt, true /* fail */)); assert (u); // Not from cache. if (bt.path () == bp) @@ -3386,6 +3439,7 @@ namespace build2 // older than the target (if it has changed since the target was // updated, then the cached data is stale). // + optional r; if ((*l)[0] == '@') { size_t p (l->find ('\'', 3)); @@ -3395,19 +3449,35 @@ namespace build2 path h (*l, 3, p - 3); path b (move (l->erase (0, p + 2))); - restart = add_unit (move (h), move (b), mt); + r = add_unit (move (h), move (b), mt); } else - restart = true; // Corrupt database? + r = true; // Corrupt database? } else - restart = add (path (move (*l)), true, mt); + r = add (path (move (*l)), true /* cache */, mt); + + if (r) + { + restart = *r; - if (restart) + if (restart) + { + update = true; + l6 ([&]{trace << "restarting (cache)";}); + break; + } + } + else { + // Trigger recompilation and mark as expected to fail. + // update = true; - l6 ([&]{trace << "restarting (cache)";}); - break; + md.deferred_failure = true; + + // Bail out early if we have deferred a failure. + // + return make_pair (auto_rmfile (), false); } } } @@ -3739,37 +3809,50 @@ namespace build2 } else { - restart = add (path (move (f)), false, pmt); - - // If the header does not exist (good_error), then - // restart must be true. Except that it is possible - // that someone running in parallel has already - // updated it. In this case we must force a restart - // since we haven't yet seen what's after this - // at-that-time-non-existent header. - // - // We also need to force the target update (normally - // done by add()). - // - if (good_error) - restart = true; - // - // And if we have updated the header (restart is - // true), then we may end up in this situation: an old - // header got included which caused the preprocessor - // to fail down the line. So if we are restarting, set - // the good error flag in case the process fails - // because of something like this (and if it is for a - // valid reason, then we will pick it up on the next - // round). - // - else if (restart) - good_error = true; + if (optional r = add (path (move (f)), + false /* cache */, + pmt)) + { + restart = *r; + + // If the header does not exist (good_error), then + // restart must be true. Except that it is possible + // that someone running in parallel has already + // updated it. In this case we must force a restart + // since we haven't yet seen what's after this + // at-that-time-non-existent header. + // + // We also need to force the target update (normally + // done by add()). + // + if (good_error) + restart = true; + // + // And if we have updated the header (restart is + // true), then we may end up in this situation: an + // old header got included which caused the + // preprocessor to fail down the line. So if we are + // restarting, set the good error flag in case the + // process fails because of something like this (and + // if it is for a valid reason, then we will pick it + // up on the next round). + // + else if (restart) + good_error = true; - if (restart) + if (restart) + { + update = true; + l6 ([&]{trace << "restarting";}); + } + } + else { + // Trigger recompilation and mark as expected to + // fail. + // update = true; - l6 ([&]{trace << "restarting";}); + md.deferred_failure = true; } } @@ -3834,17 +3917,31 @@ namespace build2 continue; } - restart = add (path (move (f)), false, pmt); + if (optional r = add (path (move (f)), + false /* cache */, + pmt)) + { + restart = *r; - if (restart) + if (restart) + { + // The same "preprocessor may fail down the line" + // logic as above. + // + good_error = true; + + update = true; + l6 ([&]{trace << "restarting";}); + break; + } + } + else { - // The same "preprocessor may fail down the line" - // logic as above. + // Trigger recompilation, mark as expected to fail, + // and bail out. // - good_error = true; - update = true; - l6 ([&]{trace << "restarting";}); + md.deferred_failure = true; break; } } @@ -3853,10 +3950,18 @@ namespace build2 } } - if (bad_error) + if (bad_error || md.deferred_failure) break; } + // Bail out early if we have deferred a failure. + // + if (md.deferred_failure) + { + is.close (); + return make_pair (auto_rmfile (), false); + } + // In case of VC, we are parsing stderr and if things go // south, we need to copy the diagnostics for the user to see. // @@ -5655,7 +5760,10 @@ namespace build2 const file& s (pr.second); const path* sp (&s.path ()); - if (pr.first) + // Force recompilation in case of a deferred failure even if nothing + // changed. + // + if (pr.first && !md.deferred_failure) { if (md.touch) { @@ -6086,7 +6194,7 @@ namespace build2 // re-using the previously preprocessed output. However, everything // points towards us needing this in the near future since with modules // we may be out of date but not needing to re-preprocess the - // translation unit (i.e., one of the imported module's has BMIs + // translation unit (i.e., one of the imported module's BMIs has // changed). // if (!ctx.dry_run) @@ -6143,6 +6251,9 @@ namespace build2 throw failed (); } + + if (md.deferred_failure) + fail << "expected error exit status from " << x_lang << " compiler"; } // Remove preprocessed file (see above). diff --git a/libbuild2/cc/compile-rule.hxx b/libbuild2/cc/compile-rule.hxx index 4ee0725..051b2e3 100644 --- a/libbuild2/cc/compile-rule.hxx +++ b/libbuild2/cc/compile-rule.hxx @@ -125,7 +125,7 @@ namespace build2 optional&, srcout_map&) const; optional - inject_header (action, file&, const file&, bool, timestamp) const; + inject_header (action, file&, const file&, timestamp, bool) const; pair extract_headers (action, const scope&, file&, linfo, -- cgit v1.1