[Openmp-commits] [PATCH] D90962: [OpenMP] Fix possible NULL dereferences

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert accepted this revision.
jdoerfert added a comment.
This revision is now accepted and ready to land.

LGTM, I left a nit inlined but feel free to ignore.

In D90962#2423630 <https://reviews.llvm.org/D90962#2423630>, @AndreyChurbanov wrote:

> In D90962#2423099 <https://reviews.llvm.org/D90962#2423099>, @jdoerfert wrote:
>
>> I like the approach. Minor nits and one question though: Why are we talking about two line numbers and not line + column? This confuses me.
>
> This is historical discrepancy. The definition of the ident_t structure in kmp.h says:
>
>   char const *psource; /**< String describing the source location.
>                        The string is composed of semi-colon separated fields
>                        which describe the source file, the function and a pair
>                        of line numbers that delimit the construct. */
>
> So the two numbers initially supposed to indicate start and end of OpenMP construct. For standalone constructs they are coincide.
>
> Then in many places I see line/column naming of the two fields. I also was under the impression that these are line/column.
> Now I see that Intel compiler sends us begin/end line of the construct, while clang sends us line/column of code block start.
>
> Not sure what to do now with this discrepancy, the begin/end line of the code block looks preferable for tools those can consume this information.
> On the other hand, such things can be hard to change...

Argh, ... OK, so the situation we are in is that the frontends are not in agreement. One thing we should avoid is to mix the comments in the runtime as well, will just cause more confusion. I'd argue we can let the frontends do what they think is right and in the runtime just say "this is a number that is either the line end or column". We always treat it the same, given that we don't interpret it anyway. Tools need to be aware of the frontend to avoid confusion. WDYT? (This would mean an action item to make sure all comments (maybe also variable names) reflect the duality.)



================
Comment at: openmp/runtime/src/kmp_itt.inl:118
         char *buff = NULL;
-        kmp_str_loc_t str_loc = __kmp_str_loc_init(loc->psource, 1);
+        kmp_str_loc_t str_loc = __kmp_str_loc_init(loc->psource, 0);
         buff = __kmp_str_format("%s$omp$parallel:%d@%s:%d:%d", str_loc.func,
----------------
AndreyChurbanov wrote:
> jdoerfert wrote:
> > Why do these inputs change?
> Along with the cleanup of unchecked returns from external calls, I also tried to not introduce extra overhead.  Looking at __kmp_str_loc_init in more details I found that it consists of two parts - parsing of locations string and forming additional structure for the file name.  This additional structure is currently used in a single place in kmp_debugger.cpp.  For all other places second parameter should be 0 in order to eliminate extra overhead including several extra malloc/free calls.
Thanks for the explanation. 

Nit: We could add a comment as we change the value:
`/* init_frame */ 0` or even better (this is C++) `/* init_frame */ false`
`init_frame` might not be the best name either :(


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90962