-
Notifications
You must be signed in to change notification settings - Fork 98
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
Fix extract_ir.py for macos. #260
Conversation
@@ -371,4 +380,5 @@ def main(argv): | |||
|
|||
|
|||
if __name__ == '__main__': | |||
multiprocessing.set_start_method('fork') |
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.
why?
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.
oh, meant to include this in the pull request description. it's because of this
abseil/abseil-py#170
basically, the default method of multiprocessing on macos is spawn instead of fork, which means the flags don't get passed around correctly. The flags are accessed within the spawned processes, so without this line macos will use default flag values for anything inside the subprocess.
compiler_opt/tools/extract_ir.py
Outdated
'Name of the section name passed to llvm-objcopy. For linux systems, the ' | ||
'default .llvmcmd is correct. On macos, one should use __LLVM,__cmdline' |
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.
Technically this is not about the OS but about the file format (for example, you might be running training on macOS but using binaries compiled for Linux), so I think it's more accurate to say "ELF" and "Mach-O" rather than "Linux" and "macOS" respectively.
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.
fixed - thanks!
For whatever reason, the section names for .llvmcmd and .llvmbc are different on macos. One problem is that macos uses macho object files instead of elf, so in addition to the section names there are segment names that need to be specified. The other problem is that regardless of the segments, the section names are different. This patch makes the segment name configurable by a flag, and gives sensible defaults for linux + documentation for macos.
3b5c7e8
to
6f0d7a5
Compare
For whatever reason, the section names for .llvmcmd and .llvmbc are different on macos. One problem is that macos uses macho object files instead of elf, so in addition to the section names there are segment names that need to be specified. The other problem is that regardless of the segments, the section names are different. This patch makes the segment name configurable by a flag, and gives sensible defaults for linux + documentation for macos.