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

[Shankar Easwaran]

On 1/9/2013 1:29 PM, Sean Silva wrote:
> + 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).
Ok. will add an appropriate error message here.
>
> +      }
> +      else {
>
> +    bool operator()(AbsoluteAtomPair& j) {
>
> +    bool operator()(const Elf_Phdr* j) const {
>
> Please follow the LLVM coding style.
Ok.
>
> -RUN: lld -flavor ld -target x86_64-linux
> +# Will fail with unersolved symbol
> +RUN: lld -flavor ld -target x86_64-linux || exit 0
This is a obscure test, I discussed with Michael on this, I think its 
just testing the arguments.
> 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.
We may want to add additional types of files, but
> +  // 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).
We dont want any atom which is owned by a different file to this. There 
are similiar things in the Reader too, that you cannot add atoms to 
Native files.

>
> +
> +  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.
This was a change that was in lld-core and moved to lld.
> +    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.
Will do.

Thanks

Shankar Easwaran
>
>


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by the Linux Foundation