[PATCH] D67310: [ELF][AArch64] Apply some NFC cleanups to AArch64ErrataFix.cpp

[Peter Smith via Phabricator via llvm-commits]

peter.smith added inline comments.


================
Comment at: ELF/AArch64ErrataFix.cpp:438
   auto isCodeMapSymbol = [](const Symbol *b) {
-    return b->getName() == "$x" || b->getName().startswith("$x.");
+    return b->getName().startswith("$x");
   };
----------------
grimar wrote:
> MaskRay wrote:
> > grimar wrote:
> > > It is not a NFC change.
> > > 
> > > This code now accepts `$xfoo`, what wasn't before.
> > This is the question I asked in https://reviews.llvm.org/D67284#inline-604486
> > 
> > I'll probably remove NFC from the title. I actually mean this change doesn't intend to make any observable behavior changes, i.e. llvm only generates `"$a"` and `"$t"`, not `"$a."` or `"$xfoo"`.
> I see. I do not know the answer here, but all other changes LGTM.
Strictly speaking $xfoo is not a mapping symbol. There is a very small chance of interpreting a user defined symbol as a mapping symbol. Probably unlikely in practice although I'd prefer to stick to the spec if it isn't too onerous to do so.  

Once init() has been passed and we've added only $x and $x. symbols to the map, we can then use startswith("$x") as all our symbols are in the form $x, $x.n, $d $d.n  

Looking at the spec below, one approach to simplify would be to use just the char 'x', 'd' or some int like 0 or 1 in the map as all we care about is whether the range of code is AArch64 or Data.   

https://developer.arm.com/docs/ihi0056/latest/elf-for-the-arm-64-bit-architecture-aarch64-abi-2019q2-documentation
- A short form that uses a dollar character and a single letter denoting the class. This form can be used when an object producer creates mapping symbols automatically. Its use minimizes string table size.
- A longer form in which the short form is extended with a period and then any sequence of characters that are legal for a symbol. This form can be used when assembler files have to be annotated manually and the assembler does not support multiple definitions of symbols.



Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D67310