[PATCH] D17392: Embed bitcode in object file (clang cc1 part)

[Steven Wu via cfe-commits]

steven_wu added a comment.

Hi Richard

Thanks for looking at the patch! Replies are inlined with the feedback.

Steven


================
Comment at: include/clang/Frontend/CodeGenOptions.def:57
@@ -56,1 +56,3 @@
+CODEGENOPT(EmbedBitcode      , 1, 0) ///< Embed LLVM IR bitcode as data.
+CODEGENOPT(EmbedMarkerOnly   , 1, 0) ///< Only create bitcode section as marker
 CODEGENOPT(EmitDeclMetadata  , 1, 0) ///< Emit special metadata indicating what
----------------
rsmith wrote:
> What is this "marker only" mode for?
marker only mode is used to speed up the compilation while still produce enough information in the final output to check if all the files containing a bitcode section.
-fembed-bitcode option will split the compilation into two stages and it adds measurable costs to compile time due to the extra process launch, the serialization and the verifier. -fembed-bitcode-marker is just a way to avoid that costs but still mark the section for the sanity check later (done by linker in our case).

================
Comment at: lib/CodeGen/BackendUtil.cpp:792
@@ +791,3 @@
+
+  // Embed command-line options.
+  ArrayRef<uint8_t> CmdData((uint8_t*)CGOpts.CmdArgs.data(),
----------------
rsmith wrote:
> As mentioned in the discussion on cfe-dev, embedding the command line and embedding bitcode appear to be completely orthogonal and separate concerns. Can you explain why it makes sense to control both with the same flag?
I thought the original discussion is that if the command line is needed for the bitcode embedding. I am happy to add option to toggle command line embedding. Here is the proposal:
-fembed-bitcode is implying -fembed-bitcode=all that embeds both bitcode and command line.
-fembed-bitcode=bitcode only embeds bitcode and no command line.
command line itself is useless so I will not provide an option to do that.

================
Comment at: lib/CodeGen/BackendUtil.cpp:793-794
@@ +792,4 @@
+  // Embed command-line options.
+  ArrayRef<uint8_t> CmdData((uint8_t*)CGOpts.CmdArgs.data(),
+                            CGOpts.CmdArgs.size());
+  llvm::Constant *CmdConstant =
----------------
rsmith wrote:
> This doesn't seem like you're saving enough information to be able to replay the build (you're missing the current working directory, for instance). Can you clarify exactly what you want to be able to do with the embedded command-line options?
>From the original discussion, I mentioned that all paths are useless for the purpose of reproducing compilation from bitcode input. Bitcode is very self-contain that it has all the debug info and other source information included. Switching current working directory will not affect the object file generated from the bitcode. Please let me know if I missed something.
On the other hand, we have some backend options that will affect the compilation result. There are few options that are not properly encoded in the bitcode (see D17394) that should be passed from command line to reproduce the exact same compilation. The end goal is for command line section is that for -fembed-bitcode, there are two stages:
1. source code -> bitcode
2. bitcode -> object file
Command line section should store all the arguments in stage 2 so it can be replayed if needed.
Without D17394, too much information is embedded in the command line section (warning flags, include paths, etc., all embedded but ignored by clang if input is bitcode). D17394 is trying to trim down the options used in stage 2.


http://reviews.llvm.org/D17392