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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev marked an inline comment as done.
ABataev added a comment.

In D65819#1627631 <https://reviews.llvm.org/D65819#1627631>, @Hahnfeld wrote:

> 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.


I don't have a small reproducer, unfortunately, only the big one. 
Here is the message from the user:

  Hi,
  I am revisiting this possible compiler bug in Clang 8.0.0.
  
  In the source code I am developing, there's a global static variable, nest::sli_neuron::recordablesMap_ put in the BSS section and it is expected to be fully initialized by the time nest::sli_neuron::sli_neuron() gets called, however in a gdb session:
  
  (gdb) p nest::sli_neuron::recordablesMap_
  Python Exception <type 'exceptions.AttributeError'> 'gdb.Type' object has no attribute 'name':
  $1 = {<std::map<Name, double (nest::sli_neuron::*)() const, std::less<Name>, std::allocator<std::pair<Name const, double (nest::sli_neuron::*)() const> > >> = std::map with 0 elements, _vptr$RecordablesMap = 0x0}
  
  this doesn't happen when -fopenmp-targets is _not_ used, is it not trivial to come up a reproducer, thus I am sending a work-in-progress report hoping someone will shed some light on this.
  
  Thanks,
  Itaru.

Another one:

  Hi,
  I am seeing a link error shown below:
  
  `.text.startup' referenced in section `.init_array.0' of /tmp/event_delivery_manager-02f392.o: defined in discarded section `.text.startup[_ZN4nest18DataSecondaryEventIdNS_16GapJunctionEventEE18supported_syn_ids_E]' of /tmp/event_delivery_manager-02f392.o
  clang-10: error: linker command failed with exit code 1 (use -v to see invocation)
  
  I am not sure how to tackle this as the part is referenced isn't what I am
  working on. I am using the latest trunk Clang 10 at this moment.

Steps to reproduce:

  Steps to produce:
  $ git clone https://https://github.com/nest/nest-simulator/
  $ mkdir /tmp/build/nest-clang-offload/
  $ cd /tmp/build/nest-clang-offload/
  $ cmake -DCMAKE_TOOLCHAIN_FILE=Platform/JURON_Clang -DCMAKE_INSTALL_PREFIX=/path/to/opt/nest-clang -Dwith-gsl=ON -Dwith-openmp=ON -Dwith-mpi=OFF -Dwith-python=OFF -Dwith-offload=ON /path/to/nest-simulator/

> 
> 
>> 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(),
----------------
Hahnfeld wrote:
> 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.
Ahh, I see. I will try to do this. I will pack all the device objects files + host object file into a resulting bundle in the saparate sections + partial link the original host code. Will check if this works.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65819