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

Andrey Churbanov via Phabricator via Openmp-commits openmp-commits at lists.llvm.org
Mon Nov 30 11:45:42 PST 2020

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.

  rG LLVM Github Monorepo



More information about the Openmp-commits mailing list