[PATCH] D11267: LLD: Add AMDGPU ELF ReaderWriter

[Simon Atanasyan]

atanasyan added a reviewer: ruiu.
atanasyan added a comment.

I added some notes and included Rui Ueyama to the reviewers list.


================
Comment at: lib/ReaderWriter/ELF/AMDGPU/AMDGPUExecutableWriter.cpp:18-33
@@ +17,17 @@
+
+void AMDGPUExecutableWriter::createImplicitFiles(
+    std::vector<std::unique_ptr<File>> &Result) {
+  // ExecutableWriter::createImplicitFiles() adds C runtime symbols that we
+  // don't need, so we use the OutputELFWriter implementation instead.
+  OutputELFWriter<ELF64LE>::createImplicitFiles(Result);
+}
+
+void AMDGPUExecutableWriter::finalizeDefaultAtomValues() {
+
+  // ExecutableWriter::finalizeDefaultAtomValues() assumes the presence of
+  // C runtime symbols.  However, since we skip the call to
+  // ExecutableWriter::createImplicitFiles(), these symbols are never added
+  // and ExectuableWriter::finalizeDefaultAtomValues() will crash if we call
+  // it.
+  OutputELFWriter<ELF64LE>::finalizeDefaultAtomValues();
+}
----------------
These methods do nothing now. I suggest to remove them from this commit and add later if necessary.

================
Comment at: lib/ReaderWriter/ELF/AMDGPU/AMDGPULinkingContext.h:23
@@ +22,3 @@
+public:
+  int getMachineType() const override { return llvm::ELF::EM_AMDGPU; }
+  AMDGPULinkingContext(llvm::Triple triple);
----------------
Usually we put constructors in the beginning of the class declarations. Other methods go after that.

================
Comment at: lib/ReaderWriter/ELF/AMDGPU/AMDGPULinkingContext.h:31
@@ +30,3 @@
+
+void setAMDGPUELFHeader(ELFHeader<ELF32LE> &elfHeader);
+void setAMDGPUELFHeader(ELFHeader<ELF64LE> &elfHeader);
----------------
Do you really need this function declaration? Below in the `AMDGPULinkingContext.cpp` you define `void setAMDGPUELFHeader(ELFHeader<ELF64LE> &elfHeader)` only.


http://reviews.llvm.org/D11267