Compile files with .incbin assembler directive locally

[asheplyakov]

A preprocessed source with .incbin assembler directive can not be compiled remotely.
As a result of remote compilation failure distcc marks the build node as unreliable (for no good reason).
This increases the build time, especially when there are only a few (2 -- 3) build nodes.
With this patch distcc checks for the .incbin directive and compiles such files locally.

To save a bit of time the check is done only on compilation error.
Files with .incbin directives are very rare, about 1 per 10000
(in Linux kernel there might be a handful of them, depending on config).
On the other hand preprocessed C sources are rather long (several MB).
Scanning ~ 10000 such files might be slower than running an extra remote compilation.

Changes since v1:

• dcc_compile_remote: check for unsupported directives only on error
• dcc_check_unsupported_directives: don't assign errno

[wtarreau]

That would be useful for the linux kernel which is now broken regarding this (I'm currently using a fallback retry for it). However, I gave it a try and it increases the build time due to the repeated use of strstr() (super expensive) and even getline(). It takes 0.5s to read through 4.2M lines of preprocessed output (small size project which builds in 6s). Dropping strstr() only cuts that time in half. I may eventually see if I can improve it to get rid of the costly getline() as well (I had done that in another project). Even fgets() would probably be sufficient because in practice we don't care about .incbin placed at the end of a very long line.

[asheplyakov]

That would be useful for the linux kernel which is now broken regarding this (I'm currently using a fallback retry for it).

That's exactly why I've written this patch in the first place.

However, I gave it a try and it increases the build time due to the repeated use of strstr() (super expensive) and even getline().

Interesting. The patch actually reduces the build time for me. By default distcc will temporarily (for 60 seconds or DISTCC_BACKOFF_PERIOD if set) disable the faulty remote node. And I've got only 3 of them, and the build takes about 2 minutes. Thus the overhead due to parsing the preprocessed sources is compensated by using all available remote nodes (and delegating all compilation to the remote nodes).

It takes 0.5s to read through 4.2M lines of preprocessed output (small size project which builds in 6s). Dropping strstr() only cuts that time in half. I may eventually see if I can improve it to get rid of the costly getline() as well (I had done that in another project). Even fgets() would probably be sufficient because in practice we don't care about .incbin placed at the end of a very long line.

The point of strstr is to avoid some false positives, like obj.incbin (), and capture some trickery like __asm__(".incbin \"" SOME_MACRO "\"")

[wtarreau]

Ah, I've been running for years with DISTCC_BACKOFF_PERIOD=0 for these reasons. Not being able to use the available CPUs was too much of a pain because most of the time it's not the server that's dead, it's just a software error. I understood that the point of these strstr() was to avoid false positives, but from what I've seen they're super rare. In the worst case we can keep one strstr() then test for both variations from the first string.

[asheplyakov]

Ah, I've been running for years with DISTCC_BACKOFF_PERIOD=0 for these reasons. Not being able to use the available CPUs was too much of a pain because most of the time it's not the server that's dead, it's just a software error. I understood that the point of these strstr() was to avoid false positives, but from what I've seen they're super rare.

In the worst case we can keep one strstr() then test for both variations from the first string.

Also we can check for .incbin (and also .include) after an error, and retry compilation locally (even if DISTCC_FALLBACK=0). In this case being slow(er) does not matter.

[wtarreau]

Yes that's what I initially thought you'd do but I thought you had a good reason for not proceeding like this (e.g. due to the way things are sequenced or anything else). Because due to the rarety of this directive, we can probably afford to see it fail once in a while.

[asheplyakov]

Yes that's what I initially thought you'd do but I thought you had a good reason for not proceeding like this (e.g. due to the way things are sequenced or anything else).

The idea was to avoid spurious error messages (and it's easier to implement).

[sourcefrog]

I feel like assigning to errno is considered bad a bad idea but I see it's already common in this tree, so, OK.

[asheplyakov]

It's not necessary, will remove it.

[asheplyakov]

Yes that's what I initially thought you'd do but I thought you had a good reason for not proceeding like this (e.g. due to the way things are sequenced or anything else).

The idea was to avoid spurious error messages (and it's easier to implement).

OK, the check for .incbin runs only on compile error now. However this does not influence the (kernel) compile time. Perhaps the overhead was negligible (for my use case) in the first place.

[sourcefrog]

It looks like check happens after attempting to compile it remotely fails. From the PR description I would have thought you could detect this first, and save a bit of time?

[asheplyakov]

It looks like check happens after attempting to compile it remotely fails.

Correct.

From the PR description I would have thought you could detect this first, and save a bit of time?

That's what I did in the first version. Reportedly it slows down the build sometimes (see the comment by @wtarreau).

Files with asm .incbin directives are very rare, like 1 in 10000 (in Linux kernel there might be a handful of them, depending on config). On the other hand preprocessed C sources are rather long (several MB). Scanning ~ 10000 such files might be slower than running an extra remote compilation.

[sourcefrog]

Ah, OK. Could you add that explanation in a comment?

[asheplyakov]

Updated the commit message and description of the pull request.