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

[Jonas Hahnfeld via Phabricator via cfe-commits]

Hahnfeld added a comment.

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

> In D65819#1627600 <https://reviews.llvm.org/D65819#1627600>, @Hahnfeld wrote:
>
> > 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.
>
>
> Unfortunately, this is the only possible solution I see. There are already 2 reports that bundled objects does not work correctly after unbundling.


Can you please again share what exactly is the problem, with a small example? I saw discussions on openmp-dev, but that project was huge, and above you were quoting a man page and hinted to global constructors.

> Plus, technically, we do not unbundle the original object file, so I would not call this unbundling at all.

Well, after this patch unbundling is strictly required to do anything with the host object.



================
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(),
----------------
ABataev wrote:
> Hahnfeld wrote:
> > Hahnfeld wrote:
> > > I think we should revert this change and just bundle the host object file as we do for all other targets.
> > To be clear: I agree that bundling + unbundling should result in the exact same object file, so the other changes seem good, just having the host object file easily accessible should be preserved.
> We just cannot use partial linking, it does not work for C++.
I'm only proposing to use partial linking such that external tools have easy access to the host object. I'm fine with storing another copy of the original host object into a section and fetch it from there during unbundling.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65819