Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Forward preprocessor definitions to the remote compiler. #42

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jpeach
Copy link
Contributor

@jpeach jpeach commented Jun 13, 2021

When doing partial local preprocessing, a remote clang compilation
still needs the preprocessor definitions from the command line so
that the remaining preprocessing can be performed correctly. Partial
preprocessing is necessary to prevent the remote compiler issuing
spurious warnings from macro expansion that would be suppressed in
a local compilation.

This means when spawning a compilation, we should always use either
LocalArgs or RemoteArgs, and the need to separately preserve
unknown arguments and conditionally restore preprocessor definitions
goes away.

This updates #37.

When doing partial local preprocessing, a remote clang compilation
still needs the preprocessor definitions from the command line so
that the remaining preprocessing can be performed correctly. Partial
preprocessing is necessary to prevent the remote compiler issuing
spurious warnings from macro expansion that would be suppressed in
a local compilation.

This means when spawning a compilation, we should always use either
`LocalArgs` or `RemoteArgs`, and the need to separately preserve
unknown arguments and conditionally restore preprocessor definitions
goes away.

This updates nelhage#37.
@jpeach
Copy link
Contributor Author

jpeach commented Jun 13, 2021

I haven't tested this with gcc, so it's possible that I've missed some compatibility implications, but it seems to me that it should always be safe to forward preprocessor definitions to the remove compiler. Worst case, they would just get ignored. 🤞

@jpeach
Copy link
Contributor Author

jpeach commented Jun 13, 2021

I haven't tested this with gcc, so it's possible that I've missed some compatibility implications, but it seems to me that it should always be safe to forward preprocessor definitions to the remove compiler. Worst case, they would just get ignored.

I think it's possible that this could break builds with -Wunused-command-line-argument

@nelhage
Copy link
Owner

nelhage commented Jun 15, 2021

Hm, this is breaking remote preprocessing because including all the arguments when detecting dependencies is problematic. e.g. it's very important that we strip any existing -M* arguments when we construct our own -M command-line.

It also OOMs if I run it at wide parallelism -- and main does not -- so I think it might be the cause of your memory issues in some way I don't fully understand yet.

@nelhage
Copy link
Owner

nelhage commented Jun 15, 2021

(When I get more time and energy to focus on Llama, it seems pretty clear to me that the next project is an integration test setup that runs the runtime inside a docker image and mocks out the Lambda invoke interface so we can actually test these things end-to-end in CI; there are just too many permutations to keep track of…)

@jpeach
Copy link
Contributor Author

jpeach commented Jun 15, 2021

Hm, this is breaking remote preprocessing because including all the arguments when detecting dependencies is problematic. e.g. it's very important that we strip any existing -M* arguments when we construct our own -M command-line.

Oh, my intention here was to forward -U and -D. I must have been a bit naive :)

@jpeach
Copy link
Contributor Author

jpeach commented Jun 19, 2021

Hm, this is breaking remote preprocessing because including all the arguments when detecting dependencies is problematic. e.g. it's very important that we strip any existing -M* arguments when we construct our own -M command-line.

AFAICT -M flags are never in Defs() and always get stripped from the remote. There is a case in the args test that covers this.

It also OOMs if I run it at wide parallelism -- and main does not -- so I think it might be the cause of your memory issues in some way I don't fully understand yet.

I added some info to #45. I think this is caused by transferring large preprocessed blobs over the net/rpc protocol, in which case I'd expect to see it whenever preprocessing is local.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants