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

[Andrey Churbanov via Phabricator via Openmp-commits]

AndreyChurbanov added a comment.

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.

The 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...



================
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,
----------------
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.


================
Comment at: openmp/runtime/src/kmp_str.cpp:328
+    *line_end = 0; // broken format of input string, cannot read number
+} // kmp_str_loc_numbers
+
----------------
jdoerfert wrote:
> No "end" comment. Also, if we want to adapt to LLVM style we should put comments on their own line and make them full sentences with punctuation etc.
I'll work on the style.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90962