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

[Xing Xue via Phabricator via libcxx-commits]

xingxue marked 11 inline comments as done.
xingxue added inline comments.


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


================
Comment at: libcxxabi/src/cxa_personality.cpp:1297
+  int number_of_states;
+  FSM_Entry table[1]; // Actually table[number_of_states]
+};
----------------
cebowleratibm wrote:
> Suggestion: use flexible array member.
Use flexible array as suggested, thanks!


================
Comment at: libcxxabi/src/cxa_personality.cpp:1308
+
+#define FSM_MAGIC 0xbeefdead
+#define FSM_MAGIC2 0xbeeedead
----------------
cebowleratibm wrote:
> 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.
Changed to use typed enum.


================
Comment at: libcxxabi/src/cxa_personality.cpp:1312
+
+#define BEGIN_CATCH_COUNT -1
+#define END_CATCH_COUNT -2
----------------
cebowleratibm wrote:
> 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.
> 
Changed to use typed enum.


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


================
Comment at: libcxxabi/src/cxa_personality.cpp:1348
+      _LIBCXXABI_TRACE_STATETAB("returned from scaler destructor\n");
+    } else {
+      _LIBCXXABI_TRACE_STATETAB("calling vector destructor\n");
----------------
cebowleratibm wrote:
> is it appropriate to assert that count won't be [-5, -1] or can those count values legitimately show up here?
When `__Invoke__Destructor()` is called, `fsmEntry->count` is greater than 0.  See line 1662.


================
Comment at: libcxxabi/src/cxa_personality.cpp:1776
+  _LIBCXXABI_TRACE_STATETAB("Entering function: %s\n\n", __func__);
+  void* newexception = __cxa_allocate_exception(sizeof(std::bad_exception));
+  __cxa_throw(newexception, (std::type_info*)&typeid(std::bad_exception), 0);
----------------
hubert.reinterpretcast wrote:
> I think a call to placement-new should be added.
Good catch, thanks!


================
Comment at: libcxxabi/src/cxa_personality.cpp:1267
+    if (StateTabDbg())                                                         \
+      fprintf(stderr, "libcxxabi: " _args);                                    \
+  } while (0)
----------------
jasonliu wrote:
> cebowleratibm wrote:
> > 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?
> Yes. It wasn't there before my comment. Xing addressed my comment. Thanks.
Including of headers for AIX is moved to the top of the file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100504