[PATCH] D110905: [llvm] Add overload to propagate errors up to caller from ExpandResponseFiles

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

What's your motivation in making this change? Do you need this for some new or out of tree caller? Would it be possible to avoid adding this as an overload and migrate existing callers to the Error return value, rather than the bool value immediately, if there aren't too many of them? (or as follow-up commits if it's too much to do in one patch?)



================
Comment at: llvm/lib/Support/CommandLine.cpp:1232-1233
       // move on.
-      // TODO: The error should be propagated up the stack.
-      llvm::consumeError(std::move(Err));
+      if (!FirstError)
+        FirstError = std::move(Err);
       AllExpanded = false;
----------------
oontvoo wrote:
> dblaikie wrote:
> > oontvoo wrote:
> > > dblaikie wrote:
> > > > Can this return immediately with th eerror, rather than setting the error and continuing to do more work?
> > > No, because we should clean up Argv and ExpandedArgv (code below) to ensure consistency.
> > > 
> > hmm - consistency with what? We could say that if this function produces an Error the argv should not be used - that the caller should error out immediately and not try to parse anything else from argv.
> fair enough!
This (changing the way argv is left on failure) only works if we actually fix the callers to do that, though - if they aren't already. Which it seems they aren't. (eg: I see one call in cl::ExpandResponseFiles that says "TODO: The error should be propagated up the stack" and another similar one in cl::ReadConfigFile.

I think it'd probably be worth doing that cleanup, if you're up for it - though it does expand the scope. If not doing that work, then yes, we probably need to preserve the argv state as it was being preserved before (at least do any tests fail/care about this? If not I might be willing to say we could break the behavior since people aren't relying on it).


================
Comment at: llvm/lib/Support/CommandLine.cpp:1267-1271
+  if (ExpandResponseFilesWithError(Saver, std::move(Tokenizer), Argv, MarkEOLs,
+                                   RelativeNames, std::move(CurrentDir),
+                                   *vfs::getRealFileSystem()))
+    return false;
+  return true;
----------------
oontvoo wrote:
> dblaikie wrote:
> > dblaikie wrote:
> > > Looks like this'll result in an assert if the function returns a non-success error, it probably needs a consumeError (or whatever it's called) in the failure case before returning false.
> > ^ this is still pending/unaddressed
> done (for real this time)
Is this error path tested? (I would've thought if it was tested, this would've been crashing/highlighted the problem earlier - so that seems to hint that maybe this isn't tested, and should be?)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110905/new/

https://reviews.llvm.org/D110905