[PATCH] D65819: [Driver][Bundler] Improve bundling of object files.

[Jonas Hahnfeld via Phabricator via cfe-commits]

Hahnfeld added a comment.

In D65819#1627036 <https://reviews.llvm.org/D65819#1627036>, @ABataev wrote:

> In D65819#1627032 <https://reviews.llvm.org/D65819#1627032>, @Hahnfeld wrote:
>
> > In D65819#1617736 <https://reviews.llvm.org/D65819#1617736>, @Hahnfeld wrote:
> >
> > > Will this patch change the ability to consume a bundled object file without calling the unbundler? Using known ELF tools on the produced object files was an important design decision and IIRC was somewhat important for using build systems that are unaware of the bundled format.
> >
> >
> > Ping.
>
>
> Missed this. We still produce correct object files, so all the tools will work with this.


I agree on a technical level that it's still a "correct" object, but not what I was looking for: The host object file will only be in the bundled section, so you cannot examine it without unbundling.

For example, with a small test file and `clang -fopenmp -fopenmp-targets=x86_64 -c test.c` I saw the following:

  $ nm test.o                                       
  0000000000000000 t .omp_offloading.requires_reg
  0000000000000000 T test
                   U __tgt_register_requires

After applying this patch, the output is empty which might be a problem in certain cases.



================
Comment at: tools/clang-offload-bundler/ClangOffloadBundler.cpp:373
 /// In order to bundle we create an IR file with the content of each section and
 /// use incremental linking to produce the resulting object. We also add section
 /// with a single byte to state the name of the component the main object file
----------------
Please update if needed.


================
Comment at: tools/clang-offload-bundler/ClangOffloadBundler.cpp:534
 
     // Do the incremental linking. We write to the output file directly. So, we
     // close it and use the name to pass down to clang.
----------------
Please update if needed.


================
Comment at: tools/clang-offload-bundler/ClangOffloadBundler.cpp:541-549
     std::vector<StringRef> ClangArgs = {"clang",
-                                        "-r",
+                                        "-c",
                                         "-target",
                                         TargetName.c_str(),
                                         "-o",
                                         OutputFileNames.front().c_str(),
-                                        InputFileNames[HostInputIndex].c_str(),
----------------
I think we should revert this change and just bundle the host object file as we do for all other targets.


================
Comment at: tools/clang-offload-bundler/ClangOffloadBundler.cpp:572
       if (Failed) {
         errs() << "error: incremental linking by external tool failed.\n";
         return true;
----------------
Please update if needed.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65819/new/

https://reviews.llvm.org/D65819