New issue
Advanced search Search tips

Issue 918259 link

Starred by 2 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

compiler_proxy exits when building assembly files with GOMA_VERIFY_OUTPUT=true

Project Member Reported by most...@vewd.com, Dec 29

Issue description

If you set GOMA_VERIFY_OUTPUT=true and build assembly (.s or .S) files, compiler_proxy fails a CHECK in CompileTask::ProcessSetup and exits.

There are a bunch of other conditions which cause tasks to fallback in CompileTask::ShouldFallback which I believe will cause similar CHECK failures.
 
Verify output is used for verifying artifacts from local compile and remote compile are the same.  However, Goma client avoid remote compile for some cases, and Goma client cannot verify the artifacts are the same.
I suppose such a case might need to be a Goma Error because developers expect remote compile but Goma client won't.
If building all assembly files locally is faster than building them remotely, then I consider it a feature/not a bug for the goma client to do so.  Turning this into a goma error would not be useful because nothing is broken and fixing it would cause a performance regression.
Re: #2
Let me step back, why you set "GOMA_VERIFY_OUTPUT=true"?
"GOMA_VERIFY_OUTPUT=true" request Goma client to verify remote compile and local compile provides the same result.  If Goma client skips remote compile, Goma client cannot verify.  Then, it might be natural to return the error.

Also, why you think showing error on non supported case brings performance regression?
> Let me step back, why you set "GOMA_VERIFY_OUTPUT=true"?

I'm trying to test our custom goma backend with a full build of chromium with GOMA_VERIFY_OUTPUT=true, and the goma client aborts when a boringssl .S file is built.

> "GOMA_VERIFY_OUTPUT=true" request Goma client to verify remote compile and local compile provides the same result.  If Goma client skips remote compile, Goma client cannot verify.  Then, it might be natural to return the error.

The goma client intentionally builds assembly files locally, so there is no error and nothing to verify in this case:
https://chromium.googlesource.com/infra/goma/client/+/f4c90513afd6e8b186b2295dfc6c1adf1efb50b0/client/compile_task.cc#3185

> Also, why you think showing error on non supported case brings performance regression?

I assume that comment above means "the assembler is faster to run locally than remotely", though I haven't benchmarked this.
(Sorry- I meant "that log entry", not "that comment".)
Hmm, I think we don't want to use goma currently for assembly files.
(Assuming that the main reason of distribution is parsing/optimizing C/C++ files is slow)

So can I ask you to change asm_prefix not to use goma for assembly file?
https://cs.chromium.org/chromium/src/build/toolchain/gcc_toolchain.gni?l=224&rcl=d6a855e015806cc9dccfd587c949cc410bcd965c
> Hmm, I think we don't want to use goma currently for assembly files.
> (Assuming that the main reason of distribution is parsing/optimizing C/C++ files is slow)

I agree.  But I think the goma client shouldn't crash if it is attempted, since that makes it harder to access goma client logs etc.

> So can I ask you to change asm_prefix not to use goma for assembly file?

Sure, I will try to investigate this tomorrow.
Can we add yet another flag like "GOMA_VERIFY_OUTPUT_IGNORE_NO_REMOTE=yes" or so to cover your case?  "GOMA_VERIFY_OUTPUT=true" sounds like Goma verifies the outputs, but for "should fallback" case, remote won't be used.  If "GOMA_VERIFY_OUTPUT=true", it sounds like users expecting the result verified, and I believe it should show error if it cannot verify.

Or making GOMA_VERIFY_OUTPUT string flag can be yet another choice. GOMA_VERIFY_OUTPUT=ignore_no_remote to support your case.

What do you think?

> I agree.  But I think the goma client shouldn't crash if it is attempted, since that makes it harder to access goma client logs etc.

I agree on Goma client should not crash for this case but we should decide the way we fix it.
I think it is not good to have yet another flag if it can be fixed in build system layer.


I am talking about semantics of GOMA_VERIFY_OUTPUT.  I think this ask Goma client to ensure remote result and local result are the same, and I believe it should show "Goma Error" if not possible to verify.

If I understand correctly, mostynb might want Goma client to ignore the case.

I thought adding yet another flag or allowing GOMA_VERIFY_OUTPUT to get string to specify ignore remote is good point of compromise.  However, if it is not good, where do you think good point of compromise?
I guess there are two possible meanings for GOMA_VERIFY_OUTPUT=true:
1) "locally verify the results of remote jobs"
2) "locally verify the results of remote jobs, and raise an error if there are jobs that can't be run remotely"

(1) makes sense if you just want to verify that remote jobs are correct.  (2) makes sense if you also want to verify that all jobs are handled remotely.

I am not so interested in the extra check in (2), maybe the warning log in https://chromium-review.googlesource.com/c/infra/goma/client/+/1392783 is sufficient?
Ok with just showing warning now, and let revisit when somebody actually confused by the meaning of GOMA_VERIFY_OUTPUT=true in the future.
Project Member

Comment 13 by bugdroid1@chromium.org, Jan 11

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/goma/client/+/3f19b33b26b5c23218d3147f48d75751dc4a22b9

commit 3f19b33b26b5c23218d3147f48d75751dc4a22b9
Author: Mostyn Bramley-Moore <mostynb@vewd.com>
Date: Thu Jan 10 02:31:53 2019

don't attempt to verify results when the task should fallback

If the task should fallback to local processing, there's no point
in trying to verify that the results match local processing- which
fails a CHECK and kills compiler_proxy anyway.

Test: build an assembly file (.s or .S extension) with
GOMA_VERIFY_OUTPUT=true and confirm that compiler_proxy is still
running.

Bug: 918259
Change-Id: Ied8ceefba773b60217074ac36f2ab9a3379b721c
[modify] https://crrev.com/3f19b33b26b5c23218d3147f48d75751dc4a22b9/client/compile_task.cc
[modify] https://crrev.com/3f19b33b26b5c23218d3147f48d75751dc4a22b9/test/simpletry.sh
[add] https://crrev.com/3f19b33b26b5c23218d3147f48d75751dc4a22b9/test/empty_assembler.s

Project Member

Comment 14 by bugdroid1@chromium.org, Jan 11

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/goma/client/+/c5610fddecbd7f97185483c2075d05c76502db82

commit c5610fddecbd7f97185483c2075d05c76502db82
Author: Yoshisato Yanagisawa <yyanagisawa@google.com>
Date: Fri Jan 11 06:14:59 2019

Sign in to add a comment