-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Add support for -S, -emit-llvm and output to stdout (-o -) #9014
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
Conversation
jgravelle-google
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woot, mostly lgtm
| }''') | ||
| run_process([PYTHON, EMCC, 'src.c', '-s', 'NO_FILESYSTEM=1']) | ||
|
|
||
| def test_dashS(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: could be dash_S to match has_dash_S? test_dashS_stdout looks like it's mixing camel and snake case to me, the underscore makes that less odd I feel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just mimicking the test_dashE_xx and test_dash_M_xxx tests below.
quantum5
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine if we don't care about .bc file generation.
emcc.py
Outdated
| has_dash_S = '-S' in newargs | ||
| if has_dash_c: | ||
| assert has_source_inputs or has_header_inputs, 'Must have source code or header inputs to use -c' | ||
| target = target_basename + '.o' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want a if '-emit-llvm' in newargs here as well to generate a .bc file?
emcc.py
Outdated
| target = target_basename + '.o' | ||
| final_suffix = '.o' | ||
| elif has_dash_S: | ||
| if '-emit-llvm' in newargs: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it's worth checking for -emit-llvm - if we do that, we may also want to check -flto and WASM_OBJECT_FILES=0 as those also affect whether we emit bitcode. But I think it might be overkill to do all that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is all for the default output filename if otherwise unspecified by the user, right? So letting this be best-effort is probably good enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regardless of WASM_OBJECT_FILES or flto settings -emit-llvm will always generate bitcode output from clang, and this is already supported in way since we pass this through from the command line.
Or are you saying the -emit-llvm might not have come from the user? .. I'll check that.
No description provided.