Page MenuHomePhabricator

[OpenMP] Consolidate error handling and debug messages in Libomptarget
ClosedPublic

Authored by jhuber6 on Aug 28 2020, 12:17 PM.

Details

Summary

This patch consolidates the error handling and messaging routines to a single file Debug.h. The goal is to simplify the error handling interface prior to adding more error handling support.

Diff Detail

Event Timeline

jhuber6 created this revision.Aug 28 2020, 12:17 PM
jhuber6 requested review of this revision.Aug 28 2020, 12:17 PM

First iteration. There's a few bugs I need to work out and I want to make sure I'm doing the correct approach.

openmp/libomptarget/include/omptargetmessage.h
1 ↗(On Diff #288670)

Is this the right location for this file? As I understand this will be visible to the rest of Clang.

openmp/libomptarget/plugins/amdgpu/src/rtl.cpp
105

I elected to just have a definition that's used for the debug message.

openmp/libomptarget/plugins/cuda/src/rtl.cpp
25–26

This is the source of a really bizarre error. This used to be static but since the symbol DebugLevel is extern in omptargetmessage.h it cannot be static here. If you print out debug messages, after Libomptarget has been unregistered you'll get some errors from CUDA saying that an error returned from cuCtxSetCurrent about three times. If DebugLevel is declared static the cuda error string will be printed out as (null). If it's not static like here it'll cause a segmentation fault. Any ideas? I feel like these errors shouldn't be emitted in the first place because they occur after Libomptarget has been unregistered.

Target CUDA RTL --> Error returned from cuCtxSetCurrent
Target CUDA RTL --> Segmentation fault (core dumped)
jdoerfert added a comment.EditedAug 28 2020, 1:00 PM

I have a lot of comments that do not directly relate to your changes but basically the code before. Anyway, while we are moving stuff we can clean it up as well.

openmp/libomptarget/include/omptargetmessage.h
1 ↗(On Diff #288670)

I dislike the omptarget prefix on headers. This is in the libomptarget subfolder, so that is redundant. I'm aware the other file has such a name, that one is bad too ;)

I would suggest something like:

  • Debug.h
  • Output.h
  • DebugInfo.h

but anything like this is better IMHO.

18 ↗(On Diff #288670)

We should add more documentation here. Also, what is "each RTL"?

24 ↗(On Diff #288670)

I would image the above macro is only needed for this include.
undef it after and introduce some newlines to make that clear.
If it is not only for the include, never mind.

51 ↗(On Diff #288670)

I'd remove this comment, move the one above into the doxygen file comment at the top of the file.

113 ↗(On Diff #288670)

Already above.

jhuber6 updated this revision to Diff 288698.Aug 28 2020, 1:50 PM
jhuber6 edited the summary of this revision. (Show Details)

Making changes

JonChesterfield added a comment.EditedAug 28 2020, 1:54 PM

Nice! Thank you, there is way too much copy and paste between these files.

The global DebugLevel/InfoLevel are nasty, but it'll be easy to introduce bugs while changing this. Keeping the non-functional code changes separate from making things better at runtime is a good strategy.

The amdgpu/impl directory contains more ad-hoc debug printing, but that should probably be incrementally deleted instead of updated to match this new scheme.

jhuber6 added inline comments.Aug 28 2020, 2:06 PM
openmp/libomptarget/include/Debug.h
44

Having these as global variables is irritating. I could replace it with a function called getDebugLevel that returns the value of the environment variable and use that in the debug macros so it wouldn't need to be called manually in the files. Could potentially store a cached result so it doesn't call getenv every time.

openmp/libomptarget/plugins/cuda/src/rtl.cpp
38–39

I'm still not sure why having DebugLevel not static causes cuGetErrorString to segfault instead of return (null).

openmp/libomptarget/plugins/ve/src/rtl.cpp
33–35

Is this supposed to be called VE?

jhuber6 added inline comments.Aug 31 2020, 5:01 AM
openmp/libomptarget/include/Debug.h
44

Is something like this an acceptable solution? It fixes the weird problems I was getting in the CUDA RTL without a static variable. The variables are still global unless I want to add a source file.

static int DebugLevel = -1; 
static int InfoLevel = -1; 

static int getInfoLevel() {
  if (InfoLevel >= 0)
    return InfoLevel;

  if (char *EnvStr = getenv("LIBOMPTARGET_INFO"))
    InfoLevel = std::stoi(EnvStr);

  return InfoLevel;
}

static int getDebugLevel() {
  if (DebugLevel >= 0)
    return DebugLevel;

  if (char *EnvStr = getenv("LIBOMPTARGET_DEBUG"))
    DebugLevel = std::stoi(EnvStr);

  return DebugLevel;
}
jdoerfert added inline comments.Aug 31 2020, 8:34 AM
openmp/libomptarget/include/Debug.h
44

if u do this, move the static declaration into the functions to make them private to the function.

jhuber6 updated this revision to Diff 288967.Aug 31 2020, 9:31 AM

Changed global debugLevel variable to a function that returns the debugLevel. Initializes once and removed the need for the RTL to check the environment variable manually as well as keeps the variables out of global scope.

I don't feel right having Debug.h shared by libomptarget and plugins especially when Debug.h is not just macro but also functions.

I don't feel right having Debug.h shared by libomptarget and plugins especially when Debug.h is not just macro but also functions.

Could you elaborate on this please. What issues are better solved by code duplication?

I don't feel right having Debug.h shared by libomptarget and plugins especially when Debug.h is not just macro but also functions.

omptarget.h already shared the DEBUGP with libomptarget and the plugins, this just moved the macros redefined in each plugin to a common header. The functions are static and inline so I would think they wouldn't cause any issues because they don't extend beyond the file.

It seems that functions are marked static so they should be OK. However, including the whole Debug.h in a plugin cpp makes it feel OK to use any function/macro from the header file. But actually only part of the macros are for the plugin. some are only for the libomptarget.

It seems that functions are marked static so they should be OK. However, including the whole Debug.h in a plugin cpp makes it feel OK to use any function/macro from the header file. But actually only part of the macros are for the plugin. some are only for the libomptarget.

I'm not sure we want to make a distinction, the point is to move to a unified debug/message model. You can choose not only the level of information but also the kind of output (text, json, ...). The messages will then be tied to the webpage via enums, that allows all plugins to emit the same message for the same thing with the same link to more information. There will certainly things that are only used in libomptarget or the plugins, but I don't see how that is any worse than duplicating the parts that are used by both.

It seems that functions are marked static so they should be OK. However, including the whole Debug.h in a plugin cpp makes it feel OK to use any function/macro from the header file. But actually only part of the macros are for the plugin. some are only for the libomptarget.

I'm not sure we want to make a distinction, the point is to move to a unified debug/message model. You can choose not only the level of information but also the kind of output (text, json, ...). The messages will then be tied to the webpage via enums, that allows all plugins to emit the same message for the same thing with the same link to more information. There will certainly things that are only used in libomptarget or the plugins, but I don't see how that is any worse than duplicating the parts that are used by both.

I didn't mean to duplicate anything. Instead you need multiple header files. One for common parts, one for libomptarget and one for plugins. The latter two both include the first one. Later you expand OFFLOAD_XXX signals, they can be added to the common file. The return signal is generated by the plugins and captured by the libomptarget. Some users may want to see only the messages captured universally by libomptarget. Some users still want to see the native error message. So the libomptarget and plugin side error handling still needs to be separated.

It seems that functions are marked static so they should be OK. However, including the whole Debug.h in a plugin cpp makes it feel OK to use any function/macro from the header file. But actually only part of the macros are for the plugin. some are only for the libomptarget.

I'm not sure we want to make a distinction, the point is to move to a unified debug/message model. You can choose not only the level of information but also the kind of output (text, json, ...). The messages will then be tied to the webpage via enums, that allows all plugins to emit the same message for the same thing with the same link to more information. There will certainly things that are only used in libomptarget or the plugins, but I don't see how that is any worse than duplicating the parts that are used by both.

I didn't mean to duplicate anything. Instead you need multiple header files. One for common parts, one for libomptarget and one for plugins. The latter two both include the first one. Later you expand OFFLOAD_XXX signals, they can be added to the common file. The return signal is generated by the plugins and captured by the libomptarget. Some users may want to see only the messages captured universally by libomptarget. Some users still want to see the native error message. So the libomptarget and plugin side error handling still needs to be separated.

I fail to see why this machinery is necessary to emit only messages from one place and not the other. I am not against a hierarchy of headers per se, but right now, and maybe also later, there seems to be little point.
I mean, we need to introduce a new env variable, actually two, that allow separate control. Once we have that we can argue about separation.
Alternatively, I would have suggested to define the "location" prior to including the debug header, e.g.,:

#define DEBUG_LOCATION "PLUGIN"
#include "Debug.h"

which we verify like:

#if DEBUG_LOCATION != "PLUGIN" and DEBUG_LOCATION != "OMPTARGET"
#error ...
#endif

At the end of the day I want to simplify things. A single location for all our debug needs sounds simpler than 1 + #plugins to me, even if we don't use all functionality at each location. If separation does not allow anything we cannot reasonably do in a single location, I doubt it provides a benefit.

This comment was removed by ye-luo.

It seems that functions are marked static so they should be OK. However, including the whole Debug.h in a plugin cpp makes it feel OK to use any function/macro from the header file. But actually only part of the macros are for the plugin. some are only for the libomptarget.

I'm not sure we want to make a distinction, the point is to move to a unified debug/message model. You can choose not only the level of information but also the kind of output (text, json, ...). The messages will then be tied to the webpage via enums, that allows all plugins to emit the same message for the same thing with the same link to more information. There will certainly things that are only used in libomptarget or the plugins, but I don't see how that is any worse than duplicating the parts that are used by both.

I didn't mean to duplicate anything. Instead you need multiple header files. One for common parts, one for libomptarget and one for plugins. The latter two both include the first one. Later you expand OFFLOAD_XXX signals, they can be added to the common file. The return signal is generated by the plugins and captured by the libomptarget. Some users may want to see only the messages captured universally by libomptarget. Some users still want to see the native error message. So the libomptarget and plugin side error handling still needs to be separated.

I fail to see why this machinery is necessary to emit only messages from one place and not the other. I am not against a hierarchy of headers per se, but right now, and maybe also later, there seems to be little point.
I mean, we need to introduce a new env variable, actually two, that allow separate control. Once we have that we can argue about separation.
Alternatively, I would have suggested to define the "location" prior to including the debug header, e.g.,:

#define DEBUG_LOCATION "PLUGIN"
#include "Debug.h"

which we verify like:

#if DEBUG_LOCATION != "PLUGIN" and DEBUG_LOCATION != "OMPTARGET"
#error ...
#endif

At the end of the day I want to simplify things. A single location for all our debug needs sounds simpler than 1 + #plugins to me, even if we don't use all functionality at each location. If separation does not allow anything we cannot reasonably do in a single location, I doubt it provides a benefit.

OK. it seems that macros are unified by DEBUG_PREFIX. So it is good for the purpose of this patch.
Long term, I think the plugin side info should be controlled separately from libomptarget side.

grokos added inline comments.Sep 1 2020, 8:26 AM
openmp/libomptarget/include/Debug.h
13

libomp target --> libomptarget

14

themessage --> the message

openmp/libomptarget/plugins/common/elf_common.c
18

This message is not applicable anymore. The DP(...) macro used to be defined in each plugin, but now with this patch this is no longer the case. The macro in defined in Debug.h, so the right message here would be "... and Debug.h has been included".

jhuber6 updated this revision to Diff 289200.Sep 1 2020, 9:35 AM

Fixed typos.

jdoerfert accepted this revision.Sep 1 2020, 11:42 AM

LGTM, one nit.

openmp/libomptarget/plugins/common/elf_common.c
18

remove "and macro DP(...) has been defined." as it always is (I think)

This revision is now accepted and ready to land.Sep 1 2020, 11:42 AM

Screwed up the commit message, whoops.