[PATCH] D68065: Propeller: LLD Support for Basic Block Sections

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/Arch/X86_64.cpp:159
+    if (is.relocations[i].offset == offset && is.relocations[i].expr != R_NONE)
+      break;
+  }
----------------
tmsriram wrote:
> MaskRay wrote:
> > MaskRay wrote:
> > > tmsriram wrote:
> > > > MaskRay wrote:
> > > > > In an old comment, I suggested
> > > > > 
> > > > > > See Relocations.cpp:scanRelocs. A better way is to sort relocations beforehand.
> > > > > 
> > > > > It is not addressed.
> > > > Could you please reconsider this? I understand what you mean here.
> > > > This code is going to change when new psABI relocations are added.  Could we re-open this then? 
> > > What is the average size of `is.relocations.size()`? It it is small in practice, the comment should mention it.
> > ```
> > for (unsigned i = is.relocations.size(); i != 0; ) {
> >   --i;
> >   if (is.relocations[i].offset == offset && is.relocations[i].expr != R_NONE)
> >     return i;
> > }
> > ```
> This won't work because this function should return the max. size when not found.  If I change that assumption, I must also change how this is consumed by its callers. Is there a reason why you want to search in the reverse direction?  I mean I will change it if  you could tell me why.
```
for (unsigned i = is.relocations.size(); i != 0; ) {
  --i;
  if (is.relocations[i].offset == offset && is.relocations[i].expr != R_NONE)
    return i;
}
return is.relocations.size();
```

An input section may have several relocations. Scanning backward can be more efficient.


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

https://reviews.llvm.org/D68065