On Thu, Jul 02, 2020 at 10:57:11AM -0400, y2s1982 . wrote: > > On Wed, Jul 01, 2020 at 10:50:44PM -0400, y2s1982 . via Gcc wrote: > > > per-process functions defined in 5.5.2. > > > I have some questions on defining or at least using some of the data > > types. > > > > The standard makes those intentionally not defined, it is up to the > > implementation to put in there whatever is needed/useful. That is the > > whole > > idea behind handles, which are pointer to these, but it is not ok for the > > debugger to make assumptions on what exactly it points to > > (similarly how e.g. with the context handles which are again pointers to > > some data in the debugger and OMPD shouldn't assume anything, just pass it > > around). > > > > I thought I understood this part, but I just need a little more nudge. I > get that standard left it > to the discretion to the implementation, and I assumed we would be the ones > to define > them. Is it okay to define the handle? Also, is the context only stored in
Yes. But only in libgompd.h, not in omp-tools.h. I.e. anything outside of libgompd.so shouldn't have a way to find out what it is and how to access it. Similarly, the ompd_address_space_context_t is something that is owned by the debugger, OMPD should have no business to look at it, only pass the pointer to the callbacks. The objects allocated by OMPD that are OMPD-internal of course may not be freed by anything but OMPD itself, and similarly something that is private to the debugger and OMPD doesn't know the size/what it contains shouldn't be freed by OMPD. > I didn't get any reply and wasn't sure if the patch was approved. The latest version was ok. > I also needed the gompd_callbacks to define the functions. > I then thought they were small enough and similar enough changes to > combine into a single patch. Should I keep them separated? Yes, commit the first patch and then send diffs against that. > Okay. I thought of having just a single return statement per function, but > I will change them. No need to have a single return, you can have as many as you want in C, it is up to the compiler to compile it. > I do have a question: as handle is considered the output of the function, > should > I still assume it is not NULL when passed in? Depends on what the standard says. If it is unspecified behavior what happens if it is NULL, then you can assume it is non-NULL and don't even need to check for NULL, if it says what should happen when NULL is passed, you should follow that. > > > + if (ret == ompd_rc_ok && (*handle).context) > > > + ret = gompd_callbacks.free_memory ((*handle).context); > > > > > +#define OMPD_TYPES_VERSION 20180906 /* YYYYMMDD Format */ > > > > Where does the 20180906 come from? > > > > Much of the ompd-types.h comes from > https://github.com/OpenMP/sources/blob/master/include/ompd-types.h. > OMPD_TYPES_VERSION was a part of it. Should I change the value as I > added more to this, or should I separate the custom definition to a > separate file > to keep the version consistent with OpenMP? Ok, keep it as is and I'll ask the author what is the reason for that. > Also, most of the formatting issues in the file were the result of the > direct > copy/paste from their code. While I did change the 8 spaces to a tab > character, > it seems there are more to fix. I will fix them. https://github.com/OpenMP/sources/ uses different formatting conventions, as I said earlier, it is ok and often required to use the same macros/function prototypes etc., but the formatting and comments should be in the GNU style. Jakub
