[PATCH] D134195: [PowerPC] XCOFF exception section support on the integrated assembler path

[Digger Lin via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
Comment at: llvm/include/llvm/MC/MCObjectWriter.h:111
+                                 unsigned FunctionSize, bool hasDebug) {
+    report_fatal_error("addExceptionEntry is only supported on "
+                       "XCOFF targets");
----------------
need a test case for this code.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:204
+  MCSymbol *Trap;
+  unsigned TrapAddress;
+  unsigned Lang;
----------------
the unsigned is 4 bytes no matter in 32bit and 64bits.  
the e_addr.e_paddr is 8bytes in 64bits. I do not think "unsigned TrapAddress" is enough to hold the data. should be change to uint64_t.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:223
+                        SectionEntry(N, Flags | XCOFF::STYP_EXCEPT) {
+    memcpy(Name, N.data(), N.size());
+  }
----------------
need assert assert(N.size() <= XCOFF::NameSize && "section name too long");  before memcpy as otherSectionEntry  constructor.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:226
+
+  ExceptionSectionEntry(ExceptionSectionEntry &&s) = default;
+
----------------
I do not  think we  need the line , the default move constructor is generated by default in this scenario.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:694
-  // when debugging is enabled.
   W.write<uint16_t>(SymbolType);
   W.write<uint8_t>(StorageClass);
----------------
why delete  the comment ?


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:809
+                                    ? SymbolIndexMap[Entry->first] + 4
+                                    : SymbolIndexMap[Entry->first] + 3);
+  } else {
----------------
add some comment to explain why +4 and +3 here.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:850
+  if (!is64Bit())
+    W.write<uint32_t>(LineNumberPointer);
+  W.write<uint32_t>(EndIndex);
----------------
nit:  still can use writeWord(LineNumberPointer); 


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1131
+  }
+  return Offset;
+}
----------------
Same reason as above. the function can change to 

 

>  unsigned EntrtNum = 0; 
>   for (auto it = ExceptionSection.ExceptionTable.begin(); it != ExceptionSection.ExceptionTable.end(); ++it) {
>     if (Symbol == it->first)
>       break;
>     EntryNum += it->second.Entries.size() + 1;
>   }
>   return EntryNum * (is64Bit() ? XCOFF::ExceptionSectionEntrySize64 : XCOFF::ExceptionSectionEntrySize32);




================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1424
+    if (is64Bit()) {
+      W.OS.write_zeros(4);
+    }
----------------
add comment to indicate this is 4 bytes padding when the entry is symbol index for 64bits.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1432
+        W.write<uint32_t>(TrapEntry.TrapAddress);
+      }
+      W.write<uint8_t>(TrapEntry.Lang);
----------------
```
      if (is64Bit()) {
        W.write<uint64_t>(TrapEntry.TrapAddress);
      } else {
        W.write<uint32_t>(TrapEntry.TrapAddress);
      }
```

change to 

```
W.wrtieWord(TrapEntry.TrapAddress);
```


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1114
+  unsigned Size = 0;
+  for (auto &SymbolEntry : ExceptionSection.ExceptionTable) {
+    Size += (is64Bit()) ? (SymbolEntry.Entries.size() + 1) *
----------------
shchenz wrote:
> Maybe we need comments here for the `+1`? The first exception entry for a function.
change the code to 

 
```
  unsigned EntryNum=0;
  
  for (auto it = ExceptionSection.ExceptionTable.begin(); it != ExceptionSection.ExceptionTable.end(); ++it)
    // The size() gets +1 to account for the initial entry containing the
    // symbol table index.
    EntryNum += it->second.Entries.size() + 1;
  
  return    EntryNum * (is64Bit() ? XCOFF::ExceptionSectionEntrySize64 : XCOFF::ExceptionSectionEntrySize32); 
```

it only need one time of multiplication ， it can improve the efficiency. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134195