[PATCH] D150870: [lld][Arm] Big Endian - Byte invariant support.

[Simi Pallipurath via Phabricator via llvm-commits]

simpal01 marked 2 inline comments as done.
simpal01 added inline comments.


================
Comment at: lld/ELF/Arch/ARM.cpp:969
+// Add a mapping symbol to the mapping symbol list.
+void elf::addArmMappingSymbols(Defined *sym) {
+  if (!isArmMapSymbol(sym) && !isDataMapSymbol(sym) && !isThumbMapSymbol(sym))
----------------
peter.smith wrote:
> simpal01 wrote:
> > peter.smith wrote:
> > > We are only adding a single symbol, whereas the name `addArmMappingSymbols` implies many symbols.
> > > 
> > > We should also be careful with using push_back here as we need to make sure that the mapping symbols are sorted. As it happens the way the code is used today push_back is fine as we always add the symbols in ascending order, but it would be easy to make a mistake in the future.
> > > 
> > > Possible solutions include:
> > > * use insert rather than push_back
> > > * sort the section map for the section after push_back
> > > * possibly delay the sorting of the mapping symbols till the first use.
> > > * change the name to push rather than add, make sure there is a comment to state that symbols must be pushed in ascending order.
> > I am sorting the mapping symbols already in this patch just before using it. I have introduced a new function named sortArmMappingSymbols which calls inside getArmMappingSymbols. By this time all the local and synthetic mapping symbols are added into the sectionmap. Sorry i could have been mentioned about it in my previous comments. 
> > I am sorting the mapping symbols already in this patch just before using it. I have introduced a new function named sortArmMappingSymbols which calls inside getArmMappingSymbols. By this time all the local and synthetic mapping symbols are added into the sectionmap. Sorry i could have been mentioned about it in my previous comments. 
> 
> OK, I missed that getMappingSymbols() is called after addArmMappingSymbols().
> 
> Although a little bit more code, one suggestion is to split getArmMappingSymbols into two functions
> addArmInputSectionMappingSymbols();
> sortArmMappingSymbols();
> 
> Which makes it clear at the call-site what is going on.
> 
> Looking at the name alone getArmMappingSymbols() doesn't hint at the side-effect of sorting the mapping symbols. Really it is addArmInputSectionMappingSymbolsThenSort()
Renamed the functions. Since the mapping symbols are sorting before first use, i hope push_back is fine to use and no need to change to insert.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150870