[llvm-commits] Followon/Preceded-By reference support in lld

[Nick Kledzik]

> 
> On Nov 6, 2012, at 1:09 PM, Shankar Easwaran wrote:
>> Hi Nick, Michael,
>> 
>> I have incorporated your review comments in the attached diff to support followon / preceded by reference in lld with
>> 
>> * 2 test cases for testing the Core functionality
>> * 1 test case for testing ReaderELF
>> 
>> Let me know if its ok to commit this change
> 
> Index: test/followon-basic.objtxt
> ===================================================================
> --- test/followon-basic.objtxt	(revision 0)
> +++ test/followon-basic.objtxt	(revision 0)
> @@ -0,0 +1,78 @@
> +# RUN: lld-core %s -dead-strip | FileCheck %s
> +
> +#
> +# Test follow on references
> +#
> +
> +---
> +atoms:
> +    - name:              A
> +      scope:             global
> +      type:              code
> +      section-choice:    custom-required
> +      section-name:      .text
> +
Please change the ELF Reader and Writer so that the default section for type "code" is ".text".   The custom-required section is intended for user supplied sections like e.g. __attribute__((section(".foobar"))) not any of the standard sections.  This will reduce the "noise" and size of many test cases.


> 
> Index: include/lld/Core/Reference.h
> ===================================================================
> --- include/lld/Core/Reference.h	(revision 167130)
> +++ include/lld/Core/Reference.h	(working copy)
> @@ -11,6 +11,8 @@
>  #define LLD_CORE_REFERENCES_H_
>  
>  #include "llvm/Support/DataTypes.h"
> +#include "llvm/ADT/StringRef.h"
> +#include <map>
>  
>  namespace lld {
>  
> @@ -33,12 +35,29 @@
>    /// Negative kind values are architecture independent.
>    typedef int32_t Kind;
>  
> +  enum {
> +    FollowOn = -1,
> +    PrecededBy = -2,
> +    LayoutBefore = -3,
> +    LayoutAfter = -4
> +  };
> +
We only need two of these four.  I was suggesting that kindLayoutBefore and kindLayoutAfter are clearer names than follow-on and preceded-by.  


>    // A value to be added to the value of a target
>    typedef int64_t Addend;
>  
>    /// What sort of reference this is.
>    virtual Kind kind() const = 0;
>  
> +  virtual StringRef kindToString() const {
> +    if (kind() < 0)
> +      return _kindToString.find(kind())->second;
> +    return "unknown";
> +  }
> +
> +  virtual int32_t stringToKind(StringRef kindString) const {
> +    return _stringToKind.find(kindString)->second;
> +  }
> +
>    /// During linking, some optimizations may change the code gen and
>    /// hence the reference kind.
>    virtual void setKind(Kind) = 0;
> @@ -63,15 +82,27 @@
>  
>  protected:
>    /// Atom is an abstract base class.  Only subclasses can access constructor.
> -  Reference() {}
> +  Reference() {
> +    _kindToString[-1] = "follow-on";
> +    _kindToString[-2] = "preceded-by";
> +    _kindToString[-3] = "layout-before";
> +    _kindToString[-4] = "layout-after";
> +    _stringToKind["follow-on"] = -1;
> +    _stringToKind["preceded-by"] = -2;    
> +    _stringToKind["layout-before"] = -3; 
> +    _stringToKind["layout-after"] = -4; 
> +  }
>  
>    /// The memory for Reference objects is always managed by the owning File
>    /// object.  Therefore, no one but the owning File object should call
>    /// delete on an Reference.  In fact, some File objects may bulk allocate
>    /// an array of References, so they cannot be individually deleted by anyone.
>    virtual ~Reference() {}
> -};
>  
> +private:
> +  std::map<int32_t, StringRef> _kindToString;
> +  std::map<StringRef, int32_t> _stringToKind;
> +};
>  
>  } // namespace lld
The mapping is the same for every Reference instance, so there is no need for a copy of the mapping in every object.  In other words, the mapping should be static.   And, for two entries a map is overkill.   Just make a static const array like was done in TestingKindMapping and walk the list looking for a match.


You also need to create a Pass which sorts atoms to ensure all "before" and "after" references are satisfied, and errors if unsatisfiable.  


On Nov 1, 2012, at 4:40 PM, Nick Kledzik wrote:
> For the core linking step, you need a bunch of yaml test cases which verify that the follow-on constraint is respected.  For example:
> a) input has A, B, C, D with follow-on from B to C, a regular reference from D to B, and mark D don't dead strip, then enable dead strip and verify output is B, C, D.
> b) input is A, B, C with following from C to B and B to A.  Verify output is C, B, A (Note, you'll need to write a pass to do the re-order).