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

[Sriraman Tallam via Phabricator via llvm-commits]

tmsriram added a comment.

In D68065#1958233 <https://reviews.llvm.org/D68065#1958233>, @MaskRay wrote:

> I have a question about `aaaaaaa.BB.foo` style labels. Are they STB_LOCAL symbol? If so,
>
> - The assembler (MC) will generally convert relocations referencing STB_LOCAL to reference STT_SECTION instead.
> - The assembler will keep them in .symtab
> - The linker will retain such labels in the .symtab section in the executable/shared object unless --discard-locals is specified.
> - `aaaa.BB.foo` will just be marker symbols in executables/shared objects: "hey, these addresses are special and are referenced by some instructions."
> - The strip tool will drop .symtab . It seems that the executables/shared objects cannot be stripped.


Acknowledged. We are aware of all these points.  We will be adding -mrelocate-with-symbols which will relocate with symbols instead of sections to perform relaxation more easily.  These symbols are used to map profiles to virtual addresses and hence the unstripped binary is  needed.   This is our first version and we are brain-storming other methods including putting it in Debug Info. Thanks.

> Are all the items above expected?





================
Comment at: lld/ELF/Arch/X86_64.cpp:159
+    if (is.relocations[i].offset == offset && is.relocations[i].expr != R_NONE)
+      break;
+  }
----------------
tmsriram wrote:
> tmsriram wrote:
> > MaskRay wrote:
> > > 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.
> > We discussed this f2f and even about sorting it and you expressed that you were alright leaving it as is, maybe you forgot.  
> Forgot to add that since this will change when the new relocations come in.
Alright, backward scanning now.


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

https://reviews.llvm.org/D68065