[PATCH] D38102: [XRay] support conditional return on PPC.

[Tim Shen via llvm-commits]

On Thu, Sep 21, 2017, 17:46 Eric Christopher via Phabricator <
reviews at reviews.llvm.org> wrote:

> echristo accepted this revision.
> echristo added a comment.
>
> Other than the readability issue, sgtm.
>
> -eric
>
>
>
> ================
> Comment at: llvm/lib/CodeGen/XRayInstrumentation.cpp:205
>      // For the architectures which don't have a single return instruction
> -    prependRetWithPatchableExit(MF, TII);
> +    prependRetWithPatchableExit(MF, TII, {false, true});
> +    break;
> ----------------
> timshen wrote:
> > dberris wrote:
> > > Looking at this, I'm inclined to suggest that using an `enum class`
> for spelling out whether to handle tail calls might be more readable. Right
> now I have to see what the meaning of 'false' in the first position is.
> Something like:
> > >
> > > ```
> > > prependRetWithPatchableExit(MF, TII,
> {InstrumentationOptions::TailCalls::IGNORE,
> InstrumentationOptions::Returns::HANDLE_ALL});
> > > ```
> > >
> > > The enum class may or may not be nested in InstrumentationOptions, so
> I'll leave that up to you.
> > FYI designated initializer (
> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0329r4.pdf)
> should be in C++20, so ultimately we'll have this:
> >   prependRetWithPatchableExit(MF, TII, {.HandleTailCalls = false,
> .HandleAllReturns = true});
> >
> > but not now. :)
> >
> > As for your suggested solution, I feel like the callee side code will be
> much longer and confusing, so I'll keep it as is.
> Bools are pretty terrible and while I agree that designated initializers
> would be better, named constants are at least better than false, true.
>
>
> ================
> Comment at: llvm/lib/CodeGen/XRayInstrumentation.cpp:45
> +
> +  InstrumentationOptions(bool HandleTailcall, bool HandleAllReturns)
> +      : HandleTailcall(HandleTailcall),
> HandleAllReturns(HandleAllReturns) {}
> ----------------
> How about an enum mask set?
>

Enum values are default constructible, so it's not better than:
  InstrumentationOptions op;
  op.x = false;
  op.y = true;

Now I prefer going back to the default ctor + assignment style.

Data members don't have to have default values. If users forgot to assign
values, ubsan will catch them.


>
> https://reviews.llvm.org/D38102
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170922/297d0460/attachment-0001.html>