Page MenuHomePhabricator

[OpenMP] Rename last file to cpp and remove LIBOMP_CFLAGS
ClosedPublic

Authored by Hahnfeld on Jul 25 2019, 8:54 AM.

Details

Summary

All other files are already C++ and the build system has always
passed '-x c++' for C files, effectively compiling them as C++.

To stay warning free we need one fix in ittnotify_static.{c,cpp}:
The variable dll_path can be written to, so it must not be const.
GCC complained with -Wcast-qual and I think it's right.

Diff Detail

Repository
rL LLVM

Event Timeline

Hahnfeld created this revision.Jul 25 2019, 8:54 AM
Herald added a project: Restricted Project. · View Herald Transcript
AndreyChurbanov accepted this revision.Jul 26 2019, 5:01 AM

LGTM.

I will contact ittnotify owners just in case. So that we might have lesser burden with possible future updates of this third-party code.

This revision is now accepted and ready to land.Jul 26 2019, 5:01 AM

I will contact ittnotify owners just in case. So that we might have lesser burden with possible future updates of this third-party code.

Thanks. Is that open-source or internal to Intel?

AndreyChurbanov added a comment.EditedJul 29 2019, 7:53 AM

I will contact ittnotify owners just in case. So that we might have lesser burden with possible future updates of this third-party code.

Thanks. Is that open-source or internal to Intel?

It is open-sourced many years ago, now under BSD license; IINM, it can be found at https://github.com/intel/IntelSEAPI/tree/master/ittnotify (though we don't update it regularly without real need);
and it is also part of Intel Tools products (e.g. Amplifier, etc.).

They already agreed to upstream the last change (removal of const). Not sure it will be possible to upstream .c --> .cpp, because they depend on many other customers. But I think this is minor issue for us.

One mode detail - the link I provided is not an "official" location of ittnotify, but just one more usage of it by SEAPI project. The updates of the ittnotify itself were published sporadically. And it will get its own stable location on githab soon (the publishing is in process).

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 30 2019, 11:36 AM

Apologies, but I think my fix for -Wcast-qual is wrong and the code is still broken. Luckily, the latest Intel Compiler still warns about it which forced me to rethink what the pointers try to achieve.

At the moment, I'm inclined to say that the cast from the array dll_path to a pointer (or more precisely, from &dll_path to const char **) not only drops qualifiers, but isn't type correct either:

#include <cstdio>

static const char var[8] = { 0, 1, 2, 3, 4, 5, 6, 7 };

int main() {
	const char **p = (const char **)&var;
	printf("%p\n", *p);
}

Compiled with the latest GCC 9.1, this gives me: 0x706050403020100
So *p is not a pointer to const char *, but it already is the array of const char and the program aborts if you try to dereference it further. This currently happens to work with dll_path because

  1. it is initialized by 0, and
  2. PATH_MAX is larger than the size of a pointer.

So &dll_path is effectively a pointer to NULL.

For the correct solution, could you point me to code that uses the dll_path_ptr member? Given that there were no problem with static const char dll_path[PATH_MAX] I guess it never assigns to *dll_path_ptr, but then I don't fully understand why it needs a pointer to a pointer...

For the correct solution, could you point me to code that uses the dll_path_ptr member? Given that there were no problem with static const char dll_path[PATH_MAX] I guess it never assigns to *dll_path_ptr, but then I don't fully understand why it needs a pointer to a pointer...

I need to consult ittnotify developers to get exact answer, they are not available this week (vacation / sick leave). I'd guess the dll_path object is never used in reality, as you pointed out. To me the type of dll_path_ptr should be "char*" instead of "const char**" in order to use memory occupied by dll_path array. I'd wait till we know the details of the use / disuse of the dll_path_ptr before making final decision.

I will contact ittnotify owners just in case. So that we might have lesser burden with possible future updates of this third-party code.

Thanks. Is that open-source or internal to Intel?

It is open-sourced many years ago, now under BSD license; IINM, it can be found at https://github.com/intel/IntelSEAPI/tree/master/ittnotify (though we don't update it regularly without real need);
and it is also part of Intel Tools products (e.g. Amplifier, etc.).

They already agreed to upstream the last change (removal of const). Not sure it will be possible to upstream .c --> .cpp, because they depend on many other customers. But I think this is minor issue for us.

One mode detail - the link I provided is not an "official" location of ittnotify, but just one more usage of it by SEAPI project. The updates of the ittnotify itself were published sporadically. And it will get its own stable location on githab soon (the publishing is in process).

Just for the record - the ittnotify API was finally published at user friendly location https://github.com/intel/ittapi (about a week ago; took a bit longer than expected for me).