[libcxx-commits] [PATCH] D100504: [libc++abi][AIX] initial patch for EH on AIX

[Chris Bowler via Phabricator via libcxx-commits]

cebowleratibm requested changes to this revision.
cebowleratibm added a comment.
This revision now requires changes to proceed.

Are there any tests planned for this patch?  We would have to commit XL binaries, or I think a better approach would be to write C code with a user defined eh fsm table.



================
Comment at: libcxxabi/src/cxa_personality.cpp:1248
+
+#include <stdio.h>
+
----------------
I think this inclusion is better at the top of the file.


================
Comment at: libcxxabi/src/cxa_personality.cpp:1277
+#define _LIBCXXABI_TRACING_STATETAB StateTabDbg()
+#endif
+
----------------
I find it a bit awkward to see the matching #ifdef
suggest: #endif // NDEBUG


================
Comment at: libcxxabi/src/cxa_personality.cpp:1297
+  int number_of_states;
+  FSM_Entry table[1]; // Actually table[number_of_states]
+};
----------------
Suggestion: use flexible array member.


================
Comment at: libcxxabi/src/cxa_personality.cpp:1308
+
+#define FSM_MAGIC 0xbeefdead
+#define FSM_MAGIC2 0xbeeedead
----------------
I don't see a compelling reason to use macros for these values.  I suggest a typed enum for FSM::magic that defines the FSM_MAGIC values as eumerators.


================
Comment at: libcxxabi/src/cxa_personality.cpp:1312
+
+#define BEGIN_CATCH_COUNT -1
+#define END_CATCH_COUNT -2
----------------
I don't see a compelling reason to use macros for these values.  I suggest a typed enum that defines these values as enumerators, then union that with a size_t member.  I think this makes the contract FSMEntry::count more clear.



================
Comment at: libcxxabi/src/cxa_personality.cpp:1318
+
+#define STATE_CHANGE_OLD                                                       \
+  0x400 // State table entry is an indirect state                              \
----------------
suggest: FSMEntry::flags should be typed enum and define these flag values as enumerators.


================
Comment at: libcxxabi/src/cxa_personality.cpp:1348
+      _LIBCXXABI_TRACE_STATETAB("returned from scaler destructor\n");
+    } else {
+      _LIBCXXABI_TRACE_STATETAB("calling vector destructor\n");
----------------
is it appropriate to assert that count won't be [-5, -1] or can those count values legitimately show up here?


================
Comment at: libcxxabi/src/cxa_personality.cpp:1402
+                          dependentExceptionHeader->primaryException) -
+                      1;
+    _LIBCXXABI_TRACE_STATETAB("exceptionObject 0x%p is a dependent, "
----------------
I don't think a line break is needed here.


================
Comment at: libcxxabi/src/cxa_personality.cpp:1267
+    if (StateTabDbg())                                                         \
+      fprintf(stderr, "libcxxabi: " _args);                                    \
+  } while (0)
----------------
jasonliu wrote:
> fprintf is a function in stdio.h. I think we should include that header, otherwise, changes might not be portable.
Isn't this on line 1248?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100504