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

[Paul Scoropan via Phabricator via llvm-commits]

pscoro marked 14 inline comments as done.
pscoro 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");
----------------
shchenz wrote:
> Format here seems strange...
I ran clang-format on these lines and no changes were made


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:794
+                       SymbolRef.getStorageClass(),
+                       (is64Bit() && ExceptionSection.isDebugEnabled) ? 3 : 2);
+
----------------
shchenz wrote:
> If we are using a map for `ExceptionTable`, we can reuse `writeSymbolEntry()` and the last `writeSymbolAuxCsectEntry()` with legacy codes. Just need to do some special handling for the last parameter `NumberOfAuxEntries` for symbol which has except entries.
Not entirely sure what you had in mind here. I can not search the exception map directly in `writeSymbolEntry` because the symbol is passed by name (stringref not MCSymbol) so I'd either have to change `writeSymbolEntry`'s function signature or find the symbol in the map beforehand (what it is doing now). Let me know what you think of the code changes here and if more changes are needed.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1216
           SymbolTableIndex += 2;
+          if (hasExceptionSection() && hasExceptEntry) {
+            if (is64Bit() && ExceptionSection.isDebugEnabled)
----------------
DiggerLin wrote:
> bool hasExceptionSection() { return (bool) getExceptionSectionSize(); } }
> and XCOFFObjectWriter::getExceptionSectionSize()  loop for all ExceptionSection.ExceptionTable.
> 
> it is low efficiency here.  I think you can use variable to calculate once at the  begin of the function.
> 
> and there are several place calll  hasExceptionSection() in the patch, maybe we can reimplement as 
> bool hasExceptionSection() { !ExceptionSection.ExceptionTable.empty();}
> 
> and there are several place calll hasExceptionSection() in the patch, maybe we can reimplement as bool hasExceptionSection() { !ExceptionSection.ExceptionTable.empty();}

Went ahead with this fix




================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1217
+          if (hasExceptionSection() && hasExceptEntry) {
+            if (is64Bit() && ExceptionSection.isDebugEnabled)
+              SymbolTableIndex += 2;
----------------
DiggerLin wrote:
> Maybe better to add some comment here to explain the following code?
Made the above comment explain the number of entries in better detail


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134195