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

[Vy Nguyen via Phabricator via llvm-commits]

oontvoo added a comment.

In D110905#3043671 <https://reviews.llvm.org/D110905#3043671>, @dblaikie wrote:

> 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?)

The motivation was to have the error on response file not found (or not readable for whatever reasons) propagated up to caller (lld-macho in my case) so that it's clearer what the problem is. (Previously, if I ran `ld64.lld @filename_with_typo.txt`, it'd complain about missing arguments, whereas the real problem was the response filename being mistyped).



================
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;
----------------
dblaikie wrote:
> 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).
I've updated the the diff to address your original concern (most of it) wrt to the "!FirstError / FirstError = " idiom.

Ultimately, I guess I prefer to keep the current behaviour rather than overhauling it and its callers (clang seems to specifically rely on this and has a bunch of tests that failed when I tried to change this).




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110905