Support for the ICVs tool-var and display-affinity-var in ompd_enumerate_icvs() and ompd_get_icv_from_scope() #75
Conversation
…ate_icvs() and ompd_get_icv_from_scope()
jinisusan
requested review from
manorom,
ilagunap,
jdelsign and
jmellorcrummey
libompd/src/omp-icv.cpp
Outdated
| return ompd_rc_stale_handle; | ||
| ompd_rc_t ret; | ||
|
|
||
| assert(callbacks && "Callback table not initialized!"); |
There was a problem hiding this comment.
As a general rule, putting asserts into library code is not good, because it will crash the tool. I think a better approach would be to return an error code to the tool. Calling assert() should be the last resort, not for recoverable error handling. I know that there are other places in the LLVM OMPD library that use assert(), but I think those should be fixed too.
libompd/src/omp-icv.cpp
Outdated
| return ompd_rc_stale_handle; | ||
| ompd_rc_t ret; | ||
|
|
||
| assert(callbacks && "Callback table not initialized!"); |
|
The changes look good to me also. Just a comment on the asserts; the reason for the asserts is that we follow the LLVM philosophy: assert liberally. From the LLVM doc: "Use the “assert” macro to its fullest. Check all of your preconditions and assumptions, you never know when a bug (not necessarily even yours) might be caught early by an assertion, which reduces debugging time dramatically." A better approach would be to allow the user to turn off asserts - LLVM provides that option: "-DLLVM_ENABLE_ASSERTIONS=On". |
IMHO, that's an OK philosophy in the compiler itself, but it's a terrible philosophy in a library intended to be used by third party tools, especially when there's a simple way to recover from the error condition. |
|
Thank you very much, John and Ignacio, for the reviews. I understand the philosophy behind removing the asserts, and I am OK with replacing the asserts with returning error codes. Ignacio, are you ok with that ? But looking at the standard, the closest I could think of is ompd_rc_error -- the others do not seem to match. This is kind of a generic error though -- Ideally, I think, there should have been a special error code for the lack of a callback table implementation in the standard. We might have to add some debug statement or so alongwith returning the error code to throw more light on why this error occurs -- but that might not be ok in a library either. I could go ahead and modify this to replace the asserts with ompd_rc_error. Replacing the asserts in the rest of the code can be taken up as a separate task. John, I have been modifying gdb to use OMPD, and I have been testing these changes using that. |
|
I'm ok with that. Yes, ompd_rc_error is the closest. I was only trying to explain why there are several places with assertions. I don't think that crashing the library because a bug occurred is bad as long as the assertion message is insightful, but if the debugger implementers prefer return error codes, I'm okay with that also. |
|
@jinisusan You are right that it would be nice if there were a more specific error code, but I think simply returning ompd_rc_error is the best option. Someone (not me) should someday find the other asserts in the LLVM OMPD library and change them to return error codes if possible. |
…ate_icvs() and ompd_get_icv_from_scope() -- changed asserts to error code returns
|
Thanks! Have made the assert related changes and made the new commit. If it looks ok, it would be great if someone can do the merge on my behalf too. |
|
LGTM, merged |
|
Thank you, Joachim ! |
Changes to provide support for the ICVs tool-var and display-affinity-var in ompd_enumerate_icvs() and ompd_get_icv_from_scope()