[llvm-commits] [lld] support for adding absolute symbols in WriterELF

[Sean Silva]

+ if (bssStartAtomIter == abse ||
+     bssEndAtomIter == abse ||
+     underScoreEndAtomIter == abse ||
+     endAtomIter == abse)
+   assert(0 && "something has gone wrong with the ELFLayout");

This message should be helpful and actually explain what invariant has
been violated. Also, use llvm_unreachable instead of assert(0) (here
and elsewhere).

+      }
+      else {

+    bool operator()(AbsoluteAtomPair& j) {

+    bool operator()(const Elf_Phdr* j) const {

Please follow the LLVM coding style.

-RUN: lld -flavor ld -target x86_64-linux
+# Will fail with unersolved symbol
+RUN: lld -flavor ld -target x86_64-linux || exit 0

Could you explain this change a bit better? Won't this always succeed
now? What is it testing? (also s/unersolved/unresolved/)

+/// \brief All atoms are owned by a File. To add linker specific atoms
+/// the atoms need to be inserted to a file called (CRuntimeFile) which
+/// are basically additional symbols required by libc and other runtime
+/// libraries part of executing a program. This class provides support
+/// for adding absolute symbols and undefined symbols
+template<llvm::support::endianness target_endianness,
+         std::size_t max_align,
+         bool is64Bits>
+class CRuntimeFile : public File {

>From the comment, it seems like this is fairly generic and is not
necessarily related to "C runtime". E.g., this provides functionality
that might be used by a linker script ("adding absolute and undefined
symbols"). Perhaps it is better to name it `LinkerInternalFile` or
something like that.

+  // cannot add atoms to C Runtime file
+  virtual void addAtom(const Atom&) {
+    llvm_unreachable("cannot add atoms to C Runtime files");
+  }

This class is marked as pure virtual in the base class, which
presumably means that a derived class must implement functionality
here in order to satisfy the contract of the base class. The fact that
this is unreachable (and hence does not satisfy that contract)
probably indicates of a design failure with the File interface (or
that this class shouldn't be a File).

+
+  auto writer = target->getWriter();
+
+  // Give writer a chance to add files
+  writer->addFiles(inputs);
+
   Resolver resolver(ro, inputs);
   resolver.resolve();
   File &merged = resolver.resultFile();

-  auto writer = target->getWriter();

What did you want reviewed here? This seems like it should be a
separate (obvious) patch that can be committed directly.

+    else
+      llvm_unreachable("Only absolute / defined atoms can be added here");

This violates the Liskov Substitution Principle since there is no such
restriction in the interface.

+    else if (atomType == Atom::definitionAbsolute) {
+      const AbsoluteAtom *absoluteAtom = dyn_cast<AbsoluteAtom>(atom);
+      _absoluteAtoms.push_back(AbsoluteAtomPair(absoluteAtom,
+                                                absoluteAtom->value()));
     }

This is an incorrect use of dyn_cast<>, which can return null. If you
know the type, then use cast<>. See
<http://llvm.org/docs/ProgrammersManual.html#the-isa-cast-and-dyn-cast-templates>.

This is also an unidiomatic use of LLVM-style RTTI. It would be better
to write it as:

    else if (const AbsoluteAtom *absoluteAtom = dyn_cast<AbsoluteAtom>(atom))
      _absoluteAtoms.push_back(AbsoluteAtomPair(absoluteAtom,
                                                absoluteAtom->value()));

The same can be said about the other use of dyn_cast<> in this function.


-- Sean Silva