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

[Amilendra Kodithuwakku via Phabricator via llvm-commits]

amilendra added inline comments.


================
Comment at: lld/ELF/Arch/ARM.cpp:934
+DenseMap<InputSection *, std::vector<const Defined *>>
+elf::getArmMappingSymbolList() {
+  DenseMap<InputSection *, std::vector<const Defined *>> sectionMap;
----------------
`getArmMappingSymbolList` is called only within this file. So you can make it `static`.


================
Comment at: lld/ELF/Arch/ARM.cpp:937
+
+  // Collect mapping symbols for every executable OutputSection.
+  for (const SymbolTableEntry &entry : in.symTab->getSymbols()) {
----------------
InputSection?


================
Comment at: lld/ELF/Arch/ARM.cpp:949
+
+  // For each IutputSection make sure the mapping symbols are in sorted in
+  // ascending order.
----------------
Nit: InputSection


================
Comment at: lld/ELF/Arch/ARM.cpp:964-971
+  for (uint64_t i = start; i < end; i += width) {
+    if (width == 4) {
+      write32le(buf + i, read32(bytes + i));
+
+    } else if (width == 2) {
+      write16le(buf + i, read16(bytes + i));
+    }
----------------
Because `width` does not change within the loop, checking it before the loop would be more efficient.
Something like
```
if (width == 4) {
  for (uint64_t i = start; i < end; i += width)
      write32le(buf + i, read32(bytes + i));
}
else if (width == 2) {
  for (uint64_t i = start; i < end; i += width) 
      write16le(buf + i, read16(bytes + i));
}

Also it would be good if the hard coded `4` and `2` can be avoided to use the `CodeState` enum. Maybe `CodeWidth` is a better name for the enum?



================
Comment at: lld/ELF/Arch/ARM.cpp:979
+// at a time. All other locations remain untouched
+void elf::endianReverseArmInstructions(InputSection *sec, uint8_t *buf) {
+  DenseMap<InputSection *, std::vector<const Defined *>> sectionMap =
----------------
I can see some refactoring opportunities in this function.
If you use something like `enum CodeState { DATA = 0, HALFWORDCODE = 2, WORDCODE = 4};` you can use 
`BytesexReverseInstructions(buf, start, end, cur_state);` instead of hard coding width such as `BytesexReverseInstructions(buf, start, end, 4);` and `BytesexReverseInstructions(buf, start, end, 2);`




================
Comment at: lld/ELF/Arch/ARM.cpp:999-1004
+    if (isThumbMapSymbol(msym))
+      new_state = HALFWORDCODE;
+    else if (isArmMapSymbol(msym))
+      new_state = WORDCODE;
+    else if (isDataMapSymbol(msym))
+      new_state = DATA;
----------------
This can be in its own function. Maybe a lambda function? That will let you rewrite the `case DATA:` block like
```
        start = msym->value;
        cur_state = function(msym);
```


================
Comment at: lld/ELF/Arch/ARM.cpp:1007-1022
+    case WORDCODE:
+      if (new_state == HALFWORDCODE || new_state == DATA) {
+        end = msym->value;
+        BytesexReverseInstructions(buf, start, end, 4);
+        cur_state = new_state;
+        start = msym->value;
+      }
----------------
These two blocks can probably be folded together.

    case WORDCODE:
    case HALFWORDCODE:
      if (new_state != cur_state) {
        end = msym->value;
        BytesexReverseInstructions(buf, start, end, cur_state);
        cur_state = new_state;
        start = msym->value;
      }
      break;


================
Comment at: lld/ELF/Arch/ARM.cpp:1032-1033
+      break;
+    default:
+      break;
+    }
----------------
```
ARM.cpp:1032:5: warning: default label in switch which covers all enumeration values [-Wcovered-switch-default]
    default:
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150870