Page MenuHomePhabricator

[Polly] [NFC] [ScopDetection] Make `polly-only-func` perform regex scop name match.
ClosedPublic

Authored by bollu on Jul 21 2017, 8:13 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

bollu created this revision.Jul 21 2017, 8:13 AM
grosser accepted this revision.Jul 21 2017, 8:54 AM

Cool!

This revision is now accepted and ready to land.Jul 21 2017, 8:54 AM
Meinersbur added inline comments.Jul 21 2017, 9:05 AM
lib/Analysis/ScopDetection.cpp
285 ↗(On Diff #107676)

[Serious] Don't use llvm_unreachable to reliably abort. In release-builds something unpredictable will happen, e.g. remove the R.isValid test altogether because there is just one outcome with defined behavior.

Thanks Michael!

lib/Analysis/ScopDetection.cpp
285 ↗(On Diff #107676)

Right. I missed this one!

bollu added inline comments.Jul 23 2017, 1:21 AM
lib/Analysis/ScopDetection.cpp
285 ↗(On Diff #107676)

What does:

remove the R.isValid test altogether because there is just one outcome with defined behavior.

mean?

If I cannot guarantee llvm_unreachable will abort, would returning false be acceptable? I really want to keep the error logging because I can forsee myself spending a good couple of minutes trying to debug this at some point in the future :).

bollu updated this revision to Diff 107810.Jul 23 2017, 2:26 AM

[NFC wrt patch] return false indicating failure of regex match if regex did not compile

bollu updated this revision to Diff 107811.Jul 23 2017, 2:26 AM
  • [NFC] diff against master.
Harbormaster completed remote builds in B8508: Diff 107811.
bollu added inline comments.Jul 23 2017, 6:22 AM
lib/Analysis/ScopDetection.cpp
285 ↗(On Diff #107676)

@Meinersbur does this look okay?

singam-sanjay added inline comments.Jul 23 2017, 8:40 AM
lib/Analysis/ScopDetection.cpp
285 ↗(On Diff #107676)

@bollu You still have llvm_unreachable before return false;. I'm not sure if that was intentional.

bollu added inline comments.Jul 24 2017, 1:24 AM
lib/Analysis/ScopDetection.cpp
285 ↗(On Diff #107676)

Yes, because I don't want it to "reliably abort", I want it to abort in debug builds.

My feeling is that the user-observable behavior of debug and non-debug builds should be indentical. If this is not the case, I would see this as a bug.

bollu added a comment.Jul 24 2017, 1:32 AM

@Meinersbur: I would like an LGTM on your part to be comfortable with upstreaming this.

bollu added a comment.Jul 24 2017, 1:33 AM

@grosser - Then is every use of llvm_unreachable a bug?

@bollu: Every use where someone can create an input that triggers it.

Meinersbur added inline comments.Jul 24 2017, 3:18 AM
lib/Analysis/ScopDetection.cpp
285 ↗(On Diff #107676)

A llvm_unreachable means that the programmer thinks that that line can under no circumstance can execute. Executing it is undefined behaviour. That the program terminates with a message is a courtesy of the Debug build. In release builds, the compiler can remove the entire branch because it thinks there is no execution with defined behaviour that executes it.

See the __builtin_unreachable section at https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html .

Relevant comments in the llvm sources:

/// LLVM_BUILTIN_UNREACHABLE - On compilers which support it, expands
/// to an expression which states that it is undefined behavior for the
/// compiler to reach this point.  Otherwise is not defined.
/// In NDEBUG builds, becomes an optimizer hint that the current location
/// is not supposed to be reachable.  On compilers that don't support
/// such hints, prints a reduced message instead.
///
/// Use this instead of assert(0).  It conveys intent more clearly and
/// allows compilers to omit some unnecessary code.

So llvm_unreachable is only a substitute for assert(false).

In C programs, use abort() to exit the program under an error condition.

The LLVM command line paser calls report_fatal_error() with malformed user input. I suggest to do the same.

bollu updated this revision to Diff 107883.Jul 24 2017, 4:36 AM
  • [NFC wrt patch] change to use report_fatal_error
bollu updated this revision to Diff 107884.Jul 24 2017, 4:43 AM
  • [NFC wrt patch] removed return false because we call a noreturn function anyway
grosser accepted this revision.Jul 24 2017, 4:45 AM
grosser added inline comments.
lib/Analysis/ScopDetection.cpp
285 ↗(On Diff #107676)

You can also drop the braces around a single statement body

This revision was automatically updated to reflect the committed changes.