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

[Alexey Bataev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL369019: [Driver][Bundler] Improve bundling of object files. 
(authored by ABataev, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D65819

Files:
  cfe/trunk/test/Driver/clang-offload-bundler.c
  cfe/trunk/tools/clang-offload-bundler/ClangOffloadBundler.cpp


Index: cfe/trunk/test/Driver/clang-offload-bundler.c
===
--- cfe/trunk/test/Driver/clang-offload-bundler.c
+++ cfe/trunk/test/Driver/clang-offload-bundler.c
@@ -231,18 +231,18 @@
 
 // RUN: clang-offload-bundler -type=o 
-targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu
 -inputs=%t.o,%t.tgt1,%t.tgt2 -outputs=%t.bundle3.o -### -dump-temporary-files 
2>&1 \
 // RUN: | FileCheck %s --check-prefix CK-OBJ-CMD
-// CK-OBJ-CMD: private constant [1 x i8] zeroinitializer, section 
"__CLANG_OFFLOAD_BUNDLE__host-[[HOST:.+]]"
+// CK-OBJ-CMD: private constant [{{[0-9]+}} x i8] c"{{.+}}", section 
"__CLANG_OFFLOAD_BUNDLE__host-[[HOST:.+]]"
 // CK-OBJ-CMD: private constant [{{[0-9]+}} x i8] c"Content of device file 
1{{.+}}", section "__CLANG_OFFLOAD_BUNDLE__openmp-powerpc64le-ibm-linux-gnu"
 // CK-OBJ-CMD: private constant [{{[0-9]+}} x i8] c"Content of device file 
2{{.+}}", section "__CLANG_OFFLOAD_BUNDLE__openmp-x86_64-pc-linux-gnu"
 // CK-OBJ-CMD: clang{{(.exe)?}}" "-r" "-target" "[[HOST]]" "-o" "{{.+}}.o" 
"{{.+}}.o" "{{.+}}.bc" "-nostdlib"
 
 // RUN: clang-offload-bundler -type=o 
-targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu
 -inputs=%t.o,%t.tgt1,%t.tgt2 -outputs=%t.bundle3.o
 // RUN: clang-offload-bundler -type=o 
-targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu
 -outputs=%t.res.o,%t.res.tgt1,%t.res.tgt2 -inputs=%t.bundle3.o -unbundle
-// RUN: diff %t.bundle3.o %t.res.o
+// RUN: diff %t.o %t.res.o
 // RUN: diff %t.tgt1 %t.res.tgt1
 // RUN: diff %t.tgt2 %t.res.tgt2
 // RUN: clang-offload-bundler -type=o 
-targets=openmp-powerpc64le-ibm-linux-gnu,host-%itanium_abi_triple,openmp-x86_64-pc-linux-gnu
 -outputs=%t.res.tgt1,%t.res.o,%t.res.tgt2 -inputs=%t.bundle3.o -unbundle
-// RUN: diff %t.bundle3.o %t.res.o
+// RUN: diff %t.o %t.res.o
 // RUN: diff %t.tgt1 %t.res.tgt1
 // RUN: diff %t.tgt2 %t.res.tgt2
 
Index: cfe/trunk/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- cfe/trunk/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ cfe/trunk/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -370,13 +370,9 @@
 /// designated name.
 ///
 /// 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
-/// (the one we are bundling into) refers to.
-///
-/// To unbundle, we use just copy the contents of the designated section. If 
the
-/// requested bundle refer to the main object file, we just copy it with no
-/// changes.
+/// use incremental linking to produce the resulting object.
+///
+/// To unbundle, we just copy the contents of the designated section.
 class ObjectFileHandler final : public FileHandler {
 
   /// The object file we are currently dealing with.
@@ -471,10 +467,7 @@
   return;
 }
 
-if (Content->size() < 2)
-  OS.write(Input.getBufferStart(), Input.getBufferSize());
-else
-  OS.write(Content->data(), Content->size());
+OS.write(Content->data(), Content->size());
   }
 
   void WriteHeader(raw_fd_ostream &OS,
@@ -592,22 +585,14 @@
 std::string SectionName = OFFLOAD_BUNDLER_MAGIC_STR;
 SectionName += CurrentTriple;
 
-// Create the constant with the content of the section. For the input we 
are
-// bundling into (the host input), this is just a place-holder, so a single
-// byte is sufficient.
-assert(HostInputIndex != ~0u && "Host input index undefined??");
-Constant *Content;
-if (NumberOfProcessedInputs == HostInputIndex + 1) {
-  uint8_t Byte[] = {0};
-  Content = ConstantDataArray::get(VMContext, Byte);
-} else
-  Content = ConstantDataArray::get(
-  VMContext, ArrayRef(reinterpret_cast(
-   Input.getBufferStart()),
-   Input.getBufferSize()));
+// Create the constant with the content of the section.
+auto *Content = ConstantDataArray::get(
+VMContext, ArrayRef(reinterpret_cast(
+ Input.getBufferStart()),
+ Input.getBufferSize()));
 
-// Create the global in the desired section. We don't want t

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

[Jonas Hahnfeld via Phabricator via cfe-commits]

Hahnfeld accepted this revision.
Hahnfeld added a comment.
This revision is now accepted and ready to land.

LG, thanks for the changes.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65819



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev updated this revision to Diff 215421.
ABataev added a comment.

Rebase


Repository:
  rC Clang

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

https://reviews.llvm.org/D65819

Files:
  test/Driver/clang-offload-bundler.c
  tools/clang-offload-bundler/ClangOffloadBundler.cpp


Index: tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -370,13 +370,9 @@
 /// designated name.
 ///
 /// 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
-/// (the one we are bundling into) refers to.
+/// use incremental linking to produce the resulting object.
 ///
-/// To unbundle, we use just copy the contents of the designated section. If 
the
-/// requested bundle refer to the main object file, we just copy it with no
-/// changes.
+/// To unbundle, we just copy the contents of the designated section.
 class ObjectFileHandler final : public FileHandler {
 
   /// The object file we are currently dealing with.
@@ -471,10 +467,7 @@
   return;
 }
 
-if (Content->size() < 2)
-  OS.write(Input.getBufferStart(), Input.getBufferSize());
-else
-  OS.write(Content->data(), Content->size());
+OS.write(Content->data(), Content->size());
   }
 
   void WriteHeader(raw_fd_ostream &OS,
@@ -592,22 +585,14 @@
 std::string SectionName = OFFLOAD_BUNDLER_MAGIC_STR;
 SectionName += CurrentTriple;
 
-// Create the constant with the content of the section. For the input we 
are
-// bundling into (the host input), this is just a place-holder, so a single
-// byte is sufficient.
-assert(HostInputIndex != ~0u && "Host input index undefined??");
-Constant *Content;
-if (NumberOfProcessedInputs == HostInputIndex + 1) {
-  uint8_t Byte[] = {0};
-  Content = ConstantDataArray::get(VMContext, Byte);
-} else
-  Content = ConstantDataArray::get(
-  VMContext, ArrayRef(reinterpret_cast(
-   Input.getBufferStart()),
-   Input.getBufferSize()));
-
-// Create the global in the desired section. We don't want these globals in
-// the symbol table, so we mark them private.
+// Create the constant with the content of the section.
+auto *Content = ConstantDataArray::get(
+VMContext, ArrayRef(reinterpret_cast(
+ Input.getBufferStart()),
+ Input.getBufferSize()));
+
+// Create the global in the desired section. We don't want these globals
+// in the symbol table, so we mark them private.
 auto *GV = new GlobalVariable(*M, Content->getType(), /*IsConstant=*/true,
   GlobalVariable::PrivateLinkage, Content);
 GV->setSection(SectionName);
Index: test/Driver/clang-offload-bundler.c
===
--- test/Driver/clang-offload-bundler.c
+++ test/Driver/clang-offload-bundler.c
@@ -231,18 +231,18 @@
 
 // RUN: clang-offload-bundler -type=o 
-targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu
 -inputs=%t.o,%t.tgt1,%t.tgt2 -outputs=%t.bundle3.o -### -dump-temporary-files 
2>&1 \
 // RUN: | FileCheck %s --check-prefix CK-OBJ-CMD
-// CK-OBJ-CMD: private constant [1 x i8] zeroinitializer, section 
"__CLANG_OFFLOAD_BUNDLE__host-[[HOST:.+]]"
+// CK-OBJ-CMD: private constant [{{[0-9]+}} x i8] c"{{.+}}", section 
"__CLANG_OFFLOAD_BUNDLE__host-[[HOST:.+]]"
 // CK-OBJ-CMD: private constant [{{[0-9]+}} x i8] c"Content of device file 
1{{.+}}", section "__CLANG_OFFLOAD_BUNDLE__openmp-powerpc64le-ibm-linux-gnu"
 // CK-OBJ-CMD: private constant [{{[0-9]+}} x i8] c"Content of device file 
2{{.+}}", section "__CLANG_OFFLOAD_BUNDLE__openmp-x86_64-pc-linux-gnu"
 // CK-OBJ-CMD: clang{{(.exe)?}}" "-r" "-target" "[[HOST]]" "-o" "{{.+}}.o" 
"{{.+}}.o" "{{.+}}.bc" "-nostdlib"
 
 // RUN: clang-offload-bundler -type=o 
-targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu
 -inputs=%t.o,%t.tgt1,%t.tgt2 -outputs=%t.bundle3.o
 // RUN: clang-offload-bundler -type=o 
-targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu
 -outputs=%t.res.o,%t.res.tgt1,%t.res.tgt2 -inputs=%t.bundle3.o -unbundle
-// RUN: diff %t.bundle3.o %t.res.o
+// RUN: diff %t.o %t.res.o
 // RUN: diff %t.tgt1 %t.res.tgt1
 // RUN: diff %t.tgt2 %t.res.tgt2
 // RUN: clang-offload-bundler -type=o 
-targets=openmp-powerpc64le-ibm-linux-gnu,host-%itanium_abi_triple,openmp-x86_64-pc-linux-gnu
 -outputs=%t.res.tgt1,%t.res.o,%t.res.tgt2 -inputs=%t.bundle3.o -unbundle
-// RUN: 

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

[Jonas Hahnfeld via Phabricator via cfe-commits]

Hahnfeld added a comment.

Please submit the test changes unrelated to the code changes in a separate 
patch!


Repository:
  rC Clang

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

https://reviews.llvm.org/D65819



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D65819#1631410 , @Hahnfeld wrote:

> The code changes look good to me, but the test doesn't pass on x86. We've 
> faced the same problem when `clang-offload-bundler` was initially committed 
> and the current testing is the best we were able to do.


I reworked the test to make it more portable, try to test it on x86


Repository:
  rC Clang

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

https://reviews.llvm.org/D65819



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev updated this revision to Diff 215399.
ABataev added a comment.

Fixed comments.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65819

Files:
  test/Driver/clang-offload-bundler.c
  test/Driver/clang-offload-bundler.c.o
  tools/clang-offload-bundler/ClangOffloadBundler.cpp

Index: tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -370,13 +370,9 @@
 /// designated name.
 ///
 /// 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
-/// (the one we are bundling into) refers to.
+/// use incremental linking to produce the resulting object.
 ///
-/// To unbundle, we use just copy the contents of the designated section. If the
-/// requested bundle refer to the main object file, we just copy it with no
-/// changes.
+/// To unbundle, we just copy the contents of the designated section.
 class ObjectFileHandler final : public FileHandler {
 
   /// The object file we are currently dealing with.
@@ -471,10 +467,7 @@
   return;
 }
 
-if (Content->size() < 2)
-  OS.write(Input.getBufferStart(), Input.getBufferSize());
-else
-  OS.write(Content->data(), Content->size());
+OS.write(Content->data(), Content->size());
   }
 
   void WriteHeader(raw_fd_ostream &OS,
@@ -592,22 +585,14 @@
 std::string SectionName = OFFLOAD_BUNDLER_MAGIC_STR;
 SectionName += CurrentTriple;
 
-// Create the constant with the content of the section. For the input we are
-// bundling into (the host input), this is just a place-holder, so a single
-// byte is sufficient.
-assert(HostInputIndex != ~0u && "Host input index undefined??");
-Constant *Content;
-if (NumberOfProcessedInputs == HostInputIndex + 1) {
-  uint8_t Byte[] = {0};
-  Content = ConstantDataArray::get(VMContext, Byte);
-} else
-  Content = ConstantDataArray::get(
-  VMContext, ArrayRef(reinterpret_cast(
-   Input.getBufferStart()),
-   Input.getBufferSize()));
-
-// Create the global in the desired section. We don't want these globals in
-// the symbol table, so we mark them private.
+// Create the constant with the content of the section.
+auto *Content = ConstantDataArray::get(
+VMContext, ArrayRef(reinterpret_cast(
+ Input.getBufferStart()),
+ Input.getBufferSize()));
+
+// Create the global in the desired section. We don't want these globals
+// in the symbol table, so we mark them private.
 auto *GV = new GlobalVariable(*M, Content->getType(), /*IsConstant=*/true,
   GlobalVariable::PrivateLinkage, Content);
 GV->setSection(SectionName);
Index: test/Driver/clang-offload-bundler.c
===
--- test/Driver/clang-offload-bundler.c
+++ test/Driver/clang-offload-bundler.c
@@ -1,16 +1,18 @@
 // REQUIRES: x86-registered-target
 // REQUIRES: powerpc-registered-target
+// REQUIRES: shell
+// UNSUPPORTED: ms-sdk
 
 //
 // Generate all the types of files we can bundle.
 //
-// RUN: %clang -O0 -target powerpc64le-ibm-linux-gnu %s -E -o %t.i
-// RUN: %clangxx -O0 -target powerpc64le-ibm-linux-gnu -x c++ %s -E -o %t.ii
-// RUN: %clang -O0 -target powerpc64le-ibm-linux-gnu %s -S -emit-llvm -o %t.ll
-// RUN: %clang -O0 -target powerpc64le-ibm-linux-gnu %s -c -emit-llvm -o %t.bc
-// RUN: %clang -O0 -target powerpc64le-ibm-linux-gnu %s -S -o %t.s
-// RUN: %clang -O0 -target powerpc64le-ibm-linux-gnu %s -c -o %t.o
-// RUN: %clang -O0 -target powerpc64le-ibm-linux-gnu %s -emit-ast -o %t.ast
+// RUN: %clang -O0 -target %itanium_abi_triple %s -E -o %t.i
+// RUN: %clangxx -O0 -target %itanium_abi_triple -x c++ %s -E -o %t.ii
+// RUN: %clang -O0 -target %itanium_abi_triple %s -S -emit-llvm -o %t.ll
+// RUN: %clang -O0 -target %itanium_abi_triple %s -c -emit-llvm -o %t.bc
+// RUN: %clang -O0 -target %itanium_abi_triple %s -S -o %t.s
+// RUN: %clang -O0 -target %itanium_abi_triple %s -c -o %t.o
+// RUN: %clang -O0 -target %itanium_abi_triple %s -emit-ast -o %t.ast
 
 //
 // Generate an empty file to help with the checks of empty files.
@@ -50,27 +52,27 @@
 //
 // Check errors.
 //
-// RUN: not clang-offload-bundler -type=i -targets=host-powerpc64le-ibm-linux-gnu,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu -inputs=%t.i,%t.tgt1,%t.tgt2 -outputs=%t.bundle.i -unbundle 2>&1 | FileCheck %s --check-prefix CK-ERR1
+// RUN: not clang-offload-bundler -type=i -targets=host-%itanium_abi_tripl

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

[Jonas Hahnfeld via Phabricator via cfe-commits]

Hahnfeld requested changes to this revision.
Hahnfeld added a comment.
This revision now requires changes to proceed.

The code changes look good to me, but the test doesn't pass on x86. We've faced 
the same problem when `clang-offload-bundler` was initially committed and the 
current testing is the best we were able to do.




Comment at: test/Driver/clang-offload-bundler.c:223-227
 // Check object bundle/unbundle. The content should be bundled into an ELF
 // section (we are using a PowerPC little-endian host which uses ELF). We
 // have an already bundled file to check the unbundle and do a dry run on the
 // bundling as it cannot be tested in all host platforms that will run these
 // tests.

This still holds: I can't partially link PPC object files on x86_64. Please 
revert the test changes to not actually perform the bundling.



Comment at: tools/clang-offload-bundler/ClangOffloadBundler.cpp:373-375
 /// 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
 /// (the one we are bundling into) refers to.

This isn't true anymore.



Comment at: tools/clang-offload-bundler/ClangOffloadBundler.cpp:377
 ///
-/// To unbundle, we use just copy the contents of the designated section. If 
the
-/// requested bundle refer to the main object file, we just copy it with no
-/// changes.
+/// To unbundle, we use just copy the contents of the designated section.
 class ObjectFileHandler final : public FileHandler {

I know this has been wrong before, but can you please fix to `we just copy` 
without `use`?


Repository:
  rC Clang

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

https://reviews.llvm.org/D65819



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev updated this revision to Diff 215214.
ABataev added a comment.

Reworked to keep partial linking to make original host object available for 
analysis without unbundling.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65819

Files:
  test/Driver/clang-offload-bundler.c
  test/Driver/clang-offload-bundler.c.o
  tools/clang-offload-bundler/ClangOffloadBundler.cpp


Index: tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -374,9 +374,7 @@
 /// with a single byte to state the name of the component the main object file
 /// (the one we are bundling into) refers to.
 ///
-/// To unbundle, we use just copy the contents of the designated section. If 
the
-/// requested bundle refer to the main object file, we just copy it with no
-/// changes.
+/// To unbundle, we use just copy the contents of the designated section.
 class ObjectFileHandler final : public FileHandler {
 
   /// The object file we are currently dealing with.
@@ -471,10 +469,7 @@
   return;
 }
 
-if (Content->size() < 2)
-  OS.write(Input.getBufferStart(), Input.getBufferSize());
-else
-  OS.write(Content->data(), Content->size());
+OS.write(Content->data(), Content->size());
   }
 
   void WriteHeader(raw_fd_ostream &OS,
@@ -592,22 +587,14 @@
 std::string SectionName = OFFLOAD_BUNDLER_MAGIC_STR;
 SectionName += CurrentTriple;
 
-// Create the constant with the content of the section. For the input we 
are
-// bundling into (the host input), this is just a place-holder, so a single
-// byte is sufficient.
-assert(HostInputIndex != ~0u && "Host input index undefined??");
-Constant *Content;
-if (NumberOfProcessedInputs == HostInputIndex + 1) {
-  uint8_t Byte[] = {0};
-  Content = ConstantDataArray::get(VMContext, Byte);
-} else
-  Content = ConstantDataArray::get(
-  VMContext, ArrayRef(reinterpret_cast(
-   Input.getBufferStart()),
-   Input.getBufferSize()));
-
-// Create the global in the desired section. We don't want these globals in
-// the symbol table, so we mark them private.
+// Create the constant with the content of the section.
+auto *Content = ConstantDataArray::get(
+VMContext, ArrayRef(reinterpret_cast(
+ Input.getBufferStart()),
+ Input.getBufferSize()));
+
+// Create the global in the desired section. We don't want these globals
+// in the symbol table, so we mark them private.
 auto *GV = new GlobalVariable(*M, Content->getType(), /*IsConstant=*/true,
   GlobalVariable::PrivateLinkage, Content);
 GV->setSection(SectionName);
Index: test/Driver/clang-offload-bundler.c
===
--- test/Driver/clang-offload-bundler.c
+++ test/Driver/clang-offload-bundler.c
@@ -229,17 +229,18 @@
 
 // RUN: clang-offload-bundler -type=o 
-targets=host-powerpc64le-ibm-linux-gnu,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu
 -inputs=%t.o,%t.tgt1,%t.tgt2 -outputs=%t.bundle3.o -### -dump-temporary-files 
2>&1 \
 // RUN: | FileCheck %s --check-prefix CK-OBJ-CMD
-// CK-OBJ-CMD: private constant [1 x i8] zeroinitializer, section 
"__CLANG_OFFLOAD_BUNDLE__host-powerpc64le-ibm-linux-gnu"
+// CK-OBJ-CMD: private constant [{{[0-9]+}} x i8] c"{{.+}}", section 
"__CLANG_OFFLOAD_BUNDLE__host-powerpc64le-ibm-linux-gnu"
 // CK-OBJ-CMD: private constant [{{[0-9]+}} x i8] c"Content of device file 
1{{.+}}", section "__CLANG_OFFLOAD_BUNDLE__openmp-powerpc64le-ibm-linux-gnu"
 // CK-OBJ-CMD: private constant [{{[0-9]+}} x i8] c"Content of device file 
2{{.+}}", section "__CLANG_OFFLOAD_BUNDLE__openmp-x86_64-pc-linux-gnu"
 // CK-OBJ-CMD: clang{{(.exe)?}}" "-r" "-target" "powerpc64le-ibm-linux-gnu" 
"-o" "{{.+}}.o" "{{.+}}.o" "{{.+}}.bc" "-nostdlib"
 
-// RUN: clang-offload-bundler -type=o 
-targets=host-powerpc64le-ibm-linux-gnu,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu
 -outputs=%t.res.o,%t.res.tgt1,%t.res.tgt2 -inputs=%s.o -unbundle
-// RUN: diff %s.o %t.res.o
+// RUN: clang-offload-bundler -type=o 
-targets=host-powerpc64le-ibm-linux-gnu,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu
 -inputs=%t.o,%t.tgt1,%t.tgt2 -outputs=%t.bundle3.o
+// RUN: clang-offload-bundler -type=o 
-targets=host-powerpc64le-ibm-linux-gnu,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu
 -outputs=%t.res.o,%t.res.tgt1,%t.res.tgt2 -inputs=%t.bundle3.o -unbundle
+// RUN: diff %t.o %t.res.o
 // RUN: diff %t.tgt1 %t.res.tgt1
 // RUN: diff %t.tgt2 %t.res.tgt2
-// RUN: clang-offload-bundler -type=o 
-targets=openmp-powerpc64le-i

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

[Alexey Bataev via cfe-commits]

I said this before that most probably this is the bug in partial linking, made 
by ld. But clang-offload-bundler is also nit quite correct here since the 
original host code is actually not unbundled. I'll fix the patch later today to 
keep the partial linking for the host object, but really unbundle it when 
required.

Best regards,
Alexey Bataev

> 14 авг. 2019 г., в 5:48, Jonas Hahnfeld via Phabricator 
>  написал(а):
> 
> Hahnfeld added a comment.
> 
> Okay, so I wasn't happy with the current explanations because I don't like 
> "fixing" an issue without understanding the problem. Here's a small 
> reproducer:
> 
>  $  cat main.cpp 
>  #include "test.h"
> 
>  int main(int argc, char *argv[]) {
>m[1] = 2;
>return 0;
>  }
>  $ cat test.h 
>  #include 
>  #include 
> 
>  template 
>  struct B {
>static std::vector v;
> 
>virtual void f() {
>  v.push_back(1);
>}
>  };
> 
>  struct C : public B {
>C() { }
>  };
> 
>  template 
>  std::vector B::v;
> 
>  extern std::map m;
>  $ cat test.cpp 
>  #include "test.h"
> 
>  std::map m;
> 
> Compiling with `clang++ -c main.cpp`, `clang++ -c test.cpp`, `clang++ main.o 
> test.o -o main` works fine and the resulting executable doesn't crash.
> Now if instead partially linking `test.o` like `ld -r test.o -o test.o.ld-r` 
> and then linking the executable with `clang++ main.o test.o.ld-r -o 
> main.ld-r` outputs a binary that crashes at execution because the constructor 
> of `std::map m` is not called.
> 
> Digging in the object files reveals the reason:
> 
>  $ readelf -S test.o | grep -n1 .init_array
>  281-   0008   WAG   0 0 8
>  282:  [138] .init_array   INIT_ARRAY     09d0
>  283-   0008   WAG   0 0 8
>  284:  [139] .rela.init_array  RELA   2460
>  285-   0018  0018   G  150   138 8
>  286:  [140] .init_array   INIT_ARRAY     09d8
>  287-   0008    WA   0 0 8
>  288:  [141] .rela.init_array  RELA   2478
>  289-   0018  0018  150   140 8
>  $ readelf -S test.o.ld-r | grep -n1 .init_array
>  279-   0078     A   0 0 4
>  280:  [137] .init_array   INIT_ARRAY     1270
>  281-   0010  0008 WAG   0 0 8
>  282:  [138] .rela.init_array  RELA   3b10
>  283-   0030  0018  IG  147   137 8
> 
> I haven't further looked into the contents of the sections, but I'd guess 
> that the first `.init_array` in `test.o` contains the constructor for 
> `template  std::vector B::v`. Because it might need to be 
> merged with other TUs (in this case, it's also present in `main.o`) it is 
> marked with a group flag (`G`). The second `.init_array` in `test.o` is 
> probably the constructor for `std::map m` and not marked with a 
> group, but `ld -r` merges these two sections into one `.init_array`, now with 
> a group. So when linking with `main.o` which also contains a constructor for 
> `template  std::vector B::v`, the linker drops the call 
> to the constructor of `std::map m`:
> 
>  $ readelf -S main | grep -n1 .init_array
>  43-   0160     A   0 0 4
>  44:  [19] .init_array   INIT_ARRAY   6d98  5d98
>  45-   0018  0008  WA   0 0 8
>  $ readelf -S main.ld-r | grep -n1 .init_array
>  43-   0160     A   0 0 4
>  44:  [19] .init_array   INIT_ARRAY   6da0  5da0
>  45-   0010  0008  WA   0 0 8
> 
> (note the difference in size of the two `.init_array` sections!)
> 
> Further notes:
> 
> - Obviously, linking in a different order like `clang++ test.o.ld-r main.o -o 
> main.ld-r2` also results in a working executable, but that's not really a 
> solution.
> - Calling `ld -Ur` doesn't change anything:
> 
>  $ ld -Ur test.o -o test.o.ld-Ur
>  $ md5sum test.o.ld-*
>  a4d5cead3209ef191d5c05de63e398de  test.o.ld-r
>  a4d5cead3209ef191d5c05de63e398de  test.o.ld-Ur
> 
> ---
> 
> Long story short: This very much looks like a bug in `ld` when using partial 
> linking. So the best thing that Clang can do to (kind of) workaround this 
> problem is ensuring that bundling + unbundling results in the bitwise-same 
> host object file. However, I think we should still use partial linking for 
> easy access to the host object file, even if we don't extract it from there 
> (other tools using it from there have to blame `ld -r`, not 
> `clang-offload-bundler`, that the partially linked object file doesn't 
> correctly call global initializers).
> 
> 
> Repository:
>  rC Cla

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

[Jonas Hahnfeld via Phabricator via cfe-commits]

Hahnfeld added a comment.

Okay, so I wasn't happy with the current explanations because I don't like 
"fixing" an issue without understanding the problem. Here's a small reproducer:

  $  cat main.cpp 
  #include "test.h"
  
  int main(int argc, char *argv[]) {
m[1] = 2;
return 0;
  }
  $ cat test.h 
  #include 
  #include 
  
  template 
  struct B {
static std::vector v;
  
virtual void f() {
  v.push_back(1);
}
  };
  
  struct C : public B {
C() { }
  };
  
  template 
  std::vector B::v;
  
  extern std::map m;
  $ cat test.cpp 
  #include "test.h"
  
  std::map m;

Compiling with `clang++ -c main.cpp`, `clang++ -c test.cpp`, `clang++ main.o 
test.o -o main` works fine and the resulting executable doesn't crash.
Now if instead partially linking `test.o` like `ld -r test.o -o test.o.ld-r` 
and then linking the executable with `clang++ main.o test.o.ld-r -o main.ld-r` 
outputs a binary that crashes at execution because the constructor of 
`std::map m` is not called.

Digging in the object files reveals the reason:

  $ readelf -S test.o | grep -n1 .init_array
  281-   0008   WAG   0 0 8
  282:  [138] .init_array   INIT_ARRAY     09d0
  283-   0008   WAG   0 0 8
  284:  [139] .rela.init_array  RELA   2460
  285-   0018  0018   G  150   138 8
  286:  [140] .init_array   INIT_ARRAY     09d8
  287-   0008    WA   0 0 8
  288:  [141] .rela.init_array  RELA   2478
  289-   0018  0018  150   140 8
  $ readelf -S test.o.ld-r | grep -n1 .init_array
  279-   0078     A   0 0 4
  280:  [137] .init_array   INIT_ARRAY     1270
  281-   0010  0008 WAG   0 0 8
  282:  [138] .rela.init_array  RELA   3b10
  283-   0030  0018  IG  147   137 8

I haven't further looked into the contents of the sections, but I'd guess that 
the first `.init_array` in `test.o` contains the constructor for `template 
 std::vector B::v`. Because it might need to be merged with 
other TUs (in this case, it's also present in `main.o`) it is marked with a 
group flag (`G`). The second `.init_array` in `test.o` is probably the 
constructor for `std::map m` and not marked with a group, but `ld -r` 
merges these two sections into one `.init_array`, now with a group. So when 
linking with `main.o` which also contains a constructor for `template  std::vector B::v`, the linker drops the call to the constructor of 
`std::map m`:

  $ readelf -S main | grep -n1 .init_array
  43-   0160     A   0 0 4
  44:  [19] .init_array   INIT_ARRAY   6d98  5d98
  45-   0018  0008  WA   0 0 8
  $ readelf -S main.ld-r | grep -n1 .init_array
  43-   0160     A   0 0 4
  44:  [19] .init_array   INIT_ARRAY   6da0  5da0
  45-   0010  0008  WA   0 0 8

(note the difference in size of the two `.init_array` sections!)

Further notes:

- Obviously, linking in a different order like `clang++ test.o.ld-r main.o -o 
main.ld-r2` also results in a working executable, but that's not really a 
solution.
- Calling `ld -Ur` doesn't change anything:

  $ ld -Ur test.o -o test.o.ld-Ur
  $ md5sum test.o.ld-*
  a4d5cead3209ef191d5c05de63e398de  test.o.ld-r
  a4d5cead3209ef191d5c05de63e398de  test.o.ld-Ur

---

Long story short: This very much looks like a bug in `ld` when using partial 
linking. So the best thing that Clang can do to (kind of) workaround this 
problem is ensuring that bundling + unbundling results in the bitwise-same host 
object file. However, I think we should still use partial linking for easy 
access to the host object file, even if we don't extract it from there (other 
tools using it from there have to blame `ld -r`, not `clang-offload-bundler`, 
that the partially linked object file doesn't correctly call global 
initializers).


Repository:
  rC Clang

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

https://reviews.llvm.org/D65819



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D65819#1627693 , @Hahnfeld wrote:

> In D65819#1627638 , @ABataev wrote:
>
> > In D65819#1627631 , @Hahnfeld 
> > wrote:
> >
> > > In D65819#1627620 , @ABataev 
> > > wrote:
> > >
> > > > In D65819#1627600 , @Hahnfeld 
> > > > wrote:
> > > >
> > > > > In D65819#1627036 , @ABataev 
> > > > > wrote:
> > > > >
> > > > > > In D65819#1627032 , 
> > > > > > @Hahnfeld wrote:
> > > > > >
> > > > > > > In 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   
> > > > >    t .omp_offloading.requires_reg
> > > > >    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  'gdb.Type' object has 
> > no attribute 'name':
> >   $1 = { > std::less, std::allocator > (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/
> >
>
>
> I've seen these reports. What has eventually led to this patch, ie where do 
> constructors come into play?


When disabled partial linking and linked the original object file instead of 
the "unbundled" one manually, the global variable is initialized properly. It 
is a global variable which must call the constructor upon program start. With 
partial linking, this constructor is not call

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

[Jonas Hahnfeld via Phabricator via cfe-commits]

Hahnfeld added a comment.

In D65819#1627638 , @ABataev wrote:

> In D65819#1627631 , @Hahnfeld wrote:
>
> > In D65819#1627620 , @ABataev wrote:
> >
> > > In D65819#1627600 , @Hahnfeld 
> > > wrote:
> > >
> > > > In D65819#1627036 , @ABataev 
> > > > wrote:
> > > >
> > > > > In D65819#1627032 , 
> > > > > @Hahnfeld wrote:
> > > > >
> > > > > > In 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   
> > > >    t .omp_offloading.requires_reg
> > > >    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  'gdb.Type' object has 
> no attribute 'name':
>   $1 = { std::less, std::allocator (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/
>


I've seen these reports. What has eventually led to this patch, ie where do 
constructors come into play?


Repository:
  rC Clang

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

https://reviews.llvm.org/D65819



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 , @Hahnfeld wrote:

> In D65819#1627620 , @ABataev wrote:
>
> > In D65819#1627600 , @Hahnfeld 
> > wrote:
> >
> > > In D65819#1627036 , @ABataev 
> > > wrote:
> > >
> > > > In D65819#1627032 , @Hahnfeld 
> > > > wrote:
> > > >
> > > > > In 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   
> > >    t .omp_offloading.requires_reg
> > >    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  'gdb.Type' object has no 
attribute 'name':
  $1 = {, 
std::allocator > >> 
= 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 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 

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

[Jonas Hahnfeld via Phabricator via cfe-commits]

Hahnfeld added a comment.

In D65819#1627620 , @ABataev wrote:

> In D65819#1627600 , @Hahnfeld wrote:
>
> > In D65819#1627036 , @ABataev wrote:
> >
> > > In D65819#1627032 , @Hahnfeld 
> > > wrote:
> > >
> > > > In 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   
> >    t .omp_offloading.requires_reg
> >    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 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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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#1627600 , @Hahnfeld wrote:

> In D65819#1627036 , @ABataev wrote:
>
> > In D65819#1627032 , @Hahnfeld 
> > wrote:
> >
> > > In 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   
>    t .omp_offloading.requires_reg
>    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. Plus, 
technically, we do not unbundle the original object file, so I would not call 
this unbundling at all.




Comment at: tools/clang-offload-bundler/ClangOffloadBundler.cpp:541-549
 std::vector ClangArgs = {"clang",
-"-r",
+"-c",
 "-target",
 TargetName.c_str(),
 "-o",
 OutputFileNames.front().c_str(),
-InputFileNames[HostInputIndex].c_str(),

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


Repository:
  rC Clang

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

https://reviews.llvm.org/D65819



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jonas Hahnfeld via Phabricator via cfe-commits]

Hahnfeld added inline comments.



Comment at: tools/clang-offload-bundler/ClangOffloadBundler.cpp:541-549
 std::vector ClangArgs = {"clang",
-"-r",
+"-c",
 "-target",
 TargetName.c_str(),
 "-o",
 OutputFileNames.front().c_str(),
-InputFileNames[HostInputIndex].c_str(),

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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65819



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jonas Hahnfeld via Phabricator via cfe-commits]

Hahnfeld added a comment.

In D65819#1627036 , @ABataev wrote:

> In D65819#1627032 , @Hahnfeld wrote:
>
> > In 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   
   t .omp_offloading.requires_reg
   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 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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D65819#1627032 , @Hahnfeld wrote:

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


Repository:
  rC Clang

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

https://reviews.llvm.org/D65819



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jonas Hahnfeld via Phabricator via cfe-commits]

Hahnfeld added a comment.

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


Repository:
  rC Clang

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

https://reviews.llvm.org/D65819



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

Ping.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65819



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

Additional note. Seems to me, it has something to do with the partial linking. 
According to ld documentation, it is recommended to use `-Ur` option for 
partial linking of the C++ object files to resolve global constructors.

  -Ur
  For anything other than C++ programs, this option is equivalent to '-r': it 
generates relocatable output--i.e., an output file that can in turn serve as 
input to ld. When linking C++ programs, `-Ur' does resolve references to 
constructors, unlike '-r'. It does not work to use '-Ur' on files that were 
themselves linked with `-Ur'; once the constructor table has been built, it 
cannot be added to. Use `-Ur' only for the last partial link, and '-r' for the 
others.

The problem I saw is exactly connected with the global constructors, which are 
not called after partial linking.
Seems to me, we partially link objects files for C++ with `-Ur` option but we 
cannot say if this the last time we perform partial linking or not (we may try 
to bundle/unbundle objects several times, especially when we'll try to support 
linking with libraries). Better not to use partial linking in bundler.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65819



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D65819#1617736 , @Hahnfeld wrote:

> Can you detail what "incorrect linking" means? AFAIK the additional sections 
> were just bloating the executable, but how do they affect correctness?
>
> 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.


It might produce a lot of "junk" sections and I saw that in some cases it may 
affect the code linking. There is a report from Itaru Kitayama and I found out 
that this oartial linking leads to the incorrectly working application. Seems 
to me, there is a problem with partial linking in LD in some cases.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65819



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jonas Hahnfeld via Phabricator via cfe-commits]

Hahnfeld added a comment.

Can you detail what "incorrect linking" means? AFAIK the additional sections 
were just bloating the executable, but how do they affect correctness?

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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65819



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev created this revision.
ABataev added reviewers: yaxunl, tra, jlebar, hfinkel.
Herald added a reviewer: jdoerfert.
Herald added a project: clang.

Previously, object files were bundled using partial linking. It resulted
in the following structure of the bundled objects:

  clang-offload-bundle
  __CLANG_OFFLOAD_BUNDLE__
  
  

But when we tried to unbundle object files, it worked correctly only for
the target objects. The host object remain bundled. It produced a lot of
junk sections in the host object files and in some cases may caused
incorrect linking.

Patch improves bundling of the object files. After this patch the
bundled object looks like this:

  clang-offload-bundle
  __CLANG_OFFLOAD_BUNDLE__
  
  __CLANG_OFFLOAD_BUNDLE__
  

With this structure we are able to unbundle the host object files too so
that fter unbundling they are the same as were before.


Repository:
  rC Clang

https://reviews.llvm.org/D65819

Files:
  test/Driver/clang-offload-bundler.c
  test/Driver/clang-offload-bundler.c.o
  tools/clang-offload-bundler/ClangOffloadBundler.cpp


Index: tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -468,10 +468,7 @@
   return;
 }
 
-if (Content->size() < 2)
-  OS.write(Input.getBufferStart(), Input.getBufferSize());
-else
-  OS.write(Content->data(), Content->size());
+OS.write(Content->data(), Content->size());
   }
 
   void WriteHeader(raw_fd_ostream &OS,
@@ -539,12 +536,11 @@
 OS.close();
 SmallString<128> TargetName = getTriple(TargetNames[HostInputIndex]);
 std::vector ClangArgs = {"clang",
-"-r",
+"-c",
 "-target",
 TargetName.c_str(),
 "-o",
 OutputFileNames.front().c_str(),
-InputFileNames[HostInputIndex].c_str(),
 BitcodeFileName.c_str(),
 "-nostdlib"};
 
@@ -593,15 +589,10 @@
 // bundling into (the host input), this is just a place-holder, so a single
 // byte is sufficient.
 assert(HostInputIndex != ~0u && "Host input index undefined??");
-Constant *Content;
-if (NumberOfProcessedInputs == HostInputIndex + 1) {
-  uint8_t Byte[] = {0};
-  Content = ConstantDataArray::get(VMContext, Byte);
-} else
-  Content = ConstantDataArray::get(
-  VMContext, ArrayRef(reinterpret_cast(
-   Input.getBufferStart()),
-   Input.getBufferSize()));
+auto *Content = ConstantDataArray::get(
+VMContext, ArrayRef(reinterpret_cast(
+ Input.getBufferStart()),
+ Input.getBufferSize()));
 
 // Create the global in the desired section. We don't want these globals in
 // the symbol table, so we mark them private.
Index: test/Driver/clang-offload-bundler.c
===
--- test/Driver/clang-offload-bundler.c
+++ test/Driver/clang-offload-bundler.c
@@ -229,17 +229,18 @@
 
 // RUN: clang-offload-bundler -type=o 
-targets=host-powerpc64le-ibm-linux-gnu,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu
 -inputs=%t.o,%t.tgt1,%t.tgt2 -outputs=%t.bundle3.o -### -dump-temporary-files 
2>&1 \
 // RUN: | FileCheck %s --check-prefix CK-OBJ-CMD
-// CK-OBJ-CMD: private constant [1 x i8] zeroinitializer, section 
"__CLANG_OFFLOAD_BUNDLE__host-powerpc64le-ibm-linux-gnu"
+// CK-OBJ-CMD: private constant [{{[0-9]+}} x i8] c"{{.+}}", section 
"__CLANG_OFFLOAD_BUNDLE__host-powerpc64le-ibm-linux-gnu"
 // CK-OBJ-CMD: private constant [{{[0-9]+}} x i8] c"Content of device file 
1{{.+}}", section "__CLANG_OFFLOAD_BUNDLE__openmp-powerpc64le-ibm-linux-gnu"
 // CK-OBJ-CMD: private constant [{{[0-9]+}} x i8] c"Content of device file 
2{{.+}}", section "__CLANG_OFFLOAD_BUNDLE__openmp-x86_64-pc-linux-gnu"
-// CK-OBJ-CMD: clang{{(.exe)?}}" "-r" "-target" "powerpc64le-ibm-linux-gnu" 
"-o" "{{.+}}.o" "{{.+}}.o" "{{.+}}.bc" "-nostdlib"
+// CK-OBJ-CMD: clang{{(.exe)?}}" "-c" "-target" "powerpc64le-ibm-linux-gnu" 
"-o" "{{.+}}.o" "{{.+}}.bc" "-nostdlib"
 
-// RUN: clang-offload-bundler -type=o 
-targets=host-powerpc64le-ibm-linux-gnu,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu
 -outputs=%t.res.o,%t.res.tgt1,%t.res.tgt2 -inputs=%s.o -unbundle
-// RUN: diff %s.o %t.res.o
+// RUN: clang-offload-bundler -type=o 
-targets=host-powerpc64le-ibm-linux-gnu,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu
 -inputs=%t.o,%t.tgt1,%t.tgt2 -outputs=%t.bundle3.o
+// RUN: clang