This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP][libomptarget] rework of fatal error reporting
ClosedPublic

Authored by AlexEichenberger on Aug 24 2018, 10:34 AM.

Diff Detail

Repository
rOMP OpenMP

Event Timeline

AlexEichenberger retitled this revision from [OpenMP][libomptarget] Bringing up to spec with respect to OMP_TARGET_OFFLOAD env var to [OpenMP][libomptarget] rework of fatal error reporting.Aug 24 2018, 10:36 AM
AlexEichenberger edited the summary of this revision. (Show Details)
gtbercea accepted this revision.Aug 24 2018, 10:38 AM

LG unless other reviewers have objections.

This revision is now accepted and ready to land.Aug 24 2018, 10:38 AM

I don't understand this: In the last review you said that we need the complicated FatalMessage and now you are reverting that?!?

I don't understand this: In the last review you said that we need the complicated FatalMessage and now you are reverting that?!?

I am reimplementing this because it needs an include that may be fragile on some platforms (D51219)

using the #def, I can avoid getting a lock as the whole printing is done in one fprintf

I don't understand this: In the last review you said that we need the complicated FatalMessage and now you are reverting that?!?

I am reimplementing this because it needs an include that may be fragile on some platforms (D51219)

Can you please elaborate on "fragile"? It's in the standard, we can just use that include!

using the #def, I can avoid getting a lock as the whole printing is done in one fprintf

(I suggested this back in https://reviews.llvm.org/D50522#1199469)

RaviNarayanaswamy added inline comments.
libomptarget/src/interface.cpp
58

What scenario would result in this.

Alexey was not happy about adding an include, and we already used the define approach for the debug messages. So we reuse the same functionality that we already used for debug.
Agreed that Johnas suggested it, and given the problem with the include, Alexey urged me to change the code to what we have here.

If you prefer the old code with the added include, we can go back to that. Just trying to make you guys happy.

@ABataev what's wrong with cstdarg?

In the end I don't have a strong opinion here (just a general dislike of too many nasty macros), but in the end it's hard to review a change without any justifaction.

libomptarget/src/private.h
53

If anything please use the do { ... } while (0) logic, that's actually a good thing for macros: https://stackoverflow.com/a/1067238

libomptarget/src/interface.cpp
58

Ravi,

its probably unlikely, just wanted it here to cover all cases.

libomptarget/src/interface.cpp
59

The reason I asked is because you have different way of reporting for tgt_disabled and tgt_default.

@ABataev what's wrong with cstdarg?

In the end I don't have a strong opinion here (just a general dislike of too many nasty macros), but in the end it's hard to review a change without any justifaction.

Just wanted to exclude some extra dependencies from the code. If you're ok with it, I'm fine too.

libomptarget/src/interface.cpp
59

ok, will make default the same way as disabled

  • put fatal message for default tgt test
AlexEichenberger marked an inline comment as done.Aug 24 2018, 5:56 PM
Hahnfeld accepted this revision.Aug 25 2018, 1:16 AM

As long as we don't need to change back to the current way of doing things next week, I think I'm also fine with the macro if you think it's better. Please address the inline comment about do { ... } while (0) before committing.

libomptarget/src/private.h
53

Ping!

  • added do while(1) for macros
AlexEichenberger marked 2 inline comments as done.Aug 26 2018, 7:07 PM
Hahnfeld requested changes to this revision.Aug 27 2018, 12:08 AM

No, that's completely different from what I asked.

This revision now requires changes to proceed.Aug 27 2018, 12:08 AM
Hahnfeld added inline comments.Aug 27 2018, 12:12 AM
libomptarget/src/private.h
53

do { ... } while (0) wasn't a typo, please see the link.

  • Please remove the outer compound statement ({ ... }).
  • Change while (1) to while (0). The idea is not to make that an infinite loop (which it is not in this case because of the exit()) but to require a semicolon after "invoking" the macro.
  • Hence remove the final ;, that needs to be at the call site.
  • Merge branch 'unpatched-master' into unpatched-master-target-offload-envvar
  • while (0) edit
libomptarget/src/private.h
53

changed

Hahnfeld accepted this revision.Aug 27 2018, 9:31 AM

LGTM. One very minor comment inline, I leave it up to you whether you want to make that change - it's more important that we unbreak the build.

libomptarget/src/private.h
51

You could probably put _str directly into the string as in FATAL_MESSAGE, but I don't think this makes a difference.

This revision is now accepted and ready to land.Aug 27 2018, 9:31 AM
This revision was automatically updated to reflect the committed changes.
Hahnfeld added inline comments.Sep 6 2018, 3:54 AM
libomptarget/src/interface.cpp
23

Fun fact: We include stdarg.h here. I'll remove this as it shouldn't be needed anymore.

Hahnfeld added inline comments.Sep 6 2018, 10:02 AM
libomptarget/src/interface.cpp
23

Done in r341563.