Endless operator-> chain causing infinite loop

[Richard Smith]

On Sun, Oct 27, 2013 at 2:10 PM, Rahul Jain <1989.rahuljain at gmail.com>wrote:

> Hi,
>
> Thanks a lot Arthur and Richard for your valuable comments. Feels great
> when a beginner is supported to such an extent.
>
> As per your comments I have reworked the patch. Here is the revised one.
> Please take a look for feedback.
>
> Index: lib/Frontend/CompilerInvocation.cpp
> ===================================================================
> --- lib/Frontend/CompilerInvocation.cpp (revision 193504)
> +++ lib/Frontend/CompilerInvocation.cpp (working copy)
> @@ -1313,6 +1313,8 @@
>    Opts.MathErrno = !Opts.OpenCL && Args.hasArg(OPT_fmath_errno);
>    Opts.InstantiationDepth =
>        getLastArgIntValue(Args, OPT_ftemplate_depth, 256, Diags);
> +  Opts.ArrowDepth =
> +      getLastArgIntValue(Args, OPT_foperator_arrow_depth, 2048, Diags);
>    Opts.ConstexprCallDepth =
>        getLastArgIntValue(Args, OPT_fconstexpr_depth, 512, Diags);
>    Opts.ConstexprStepLimit =
> Index: lib/Driver/Tools.cpp
> ===================================================================
> --- lib/Driver/Tools.cpp (revision 193504)
> +++ lib/Driver/Tools.cpp (working copy)
> @@ -2802,6 +2802,11 @@
>      CmdArgs.push_back(A->getValue());
>    }
>
> +  if (Arg *A = Args.getLastArg(options::OPT_foperator_arrow_depth_EQ)) {
> +    CmdArgs.push_back("-foperator-arrow-depth");
> +    CmdArgs.push_back(A->getValue());
> +  }
> +
>    if (Arg *A = Args.getLastArg(options::OPT_fconstexpr_depth_EQ)) {
>      CmdArgs.push_back("-fconstexpr-depth");
>      CmdArgs.push_back(A->getValue());
> Index: lib/Sema/SemaExprCXX.cpp
> ===================================================================
> --- lib/Sema/SemaExprCXX.cpp (revision 193504)
> +++ lib/Sema/SemaExprCXX.cpp (working copy)
> @@ -5189,7 +5189,17 @@
>      SmallVector<SourceLocation, 8> Locations;
>      CTypes.insert(Context.getCanonicalType(BaseType));
>
> +    unsigned int Arrow_instantiation_count = 0;
>

This should be CamelCase, without underscores.


>      while (BaseType->isRecordType()) {
> +      Arrow_instantiation_count++;
> +      if (Arrow_instantiation_count >= getLangOpts().ArrowDepth) {
> + Diag(OpLoc, diag::err_arrow_instantiation_depth_exceeded)
> +  << getLangOpts().ArrowDepth << Base->getSourceRange();
> + Diag(OpLoc, diag::note_arrow_instantiation_depth)
> +  << getLangOpts().ArrowDepth;
> + return ExprError();
>

The body of this 'if' should be indented by two more spaces.


> +      }
> +
>        Result = BuildOverloadedArrowExpr(
>            S, Base, OpLoc,
>            // When in a template specialization and on the first loop
> iteration,
> Index: include/clang/Basic/LangOptions.def
> ===================================================================
> --- include/clang/Basic/LangOptions.def (revision 193504)
> +++ include/clang/Basic/LangOptions.def (working copy)
> @@ -160,6 +160,8 @@
>  ENUM_LANGOPT(SignedOverflowBehavior, SignedOverflowBehaviorTy, 2,
> SOB_Undefined,
>               "signed integer overflow handling")
>
> +BENIGN_LANGOPT(ArrowDepth, 32, 2048,
> +               "maximum number of operator->s to follow")
>  BENIGN_LANGOPT(InstantiationDepth, 32, 256,
>                 "maximum template instantiation depth")
>  BENIGN_LANGOPT(ConstexprCallDepth, 32, 512,
> Index: include/clang/Basic/DiagnosticSemaKinds.td
> ===================================================================
> --- include/clang/Basic/DiagnosticSemaKinds.td (revision 193504)
> +++ include/clang/Basic/DiagnosticSemaKinds.td (working copy)
> @@ -2569,6 +2569,11 @@
>  // can handle that case properly.
>  def note_ovl_candidate_non_deduced_mismatch_qualified : Note<
>      "candidate template ignored: could not match %q0 against %q1">;
> +def err_arrow_instantiation_depth_exceeded : Error<
> +    "arrow operator instantiation exceeded maximum depth of %0">;
> +def note_arrow_instantiation_depth : Note<
> +    "use -foperator-arrow-depth=N to increase arrow operator
> instantiation "
> +    "depth">;
>

Please don't use the word 'instantiation' here.

Have you considered including notes pointing at the sequence of operator->
functions we called, and the corresponding types? This might not be obvious.


>  // Note that we don't treat templates differently for this diagnostic.
>  def note_ovl_candidate_arity : Note<"candidate "
> Index: include/clang/Driver/CC1Options.td
> ===================================================================
> --- include/clang/Driver/CC1Options.td (revision 193504)
> +++ include/clang/Driver/CC1Options.td (working copy)
> @@ -442,6 +442,8 @@
>    HelpText<"Default type visibility">;
>  def ftemplate_depth : Separate<["-"], "ftemplate-depth">,
>    HelpText<"Maximum depth of recursive template instantiation">;
> +def foperator_arrow_depth : Separate<["-"], "foperator-arrow-depth">,
> +  HelpText<"Maximum depth of arrow operator instantiation">;
>  def fconstexpr_depth : Separate<["-"], "fconstexpr-depth">,
>    HelpText<"Maximum depth of recursive constexpr function calls">;
>  def fconstexpr_steps : Separate<["-"], "fconstexpr-steps">,
> Index: include/clang/Driver/Options.td
> ===================================================================
> --- include/clang/Driver/Options.td (revision 193504)
> +++ include/clang/Driver/Options.td (working copy)
> @@ -775,6 +775,8 @@
>  def ftemplate_depth_ : Joined<["-"], "ftemplate-depth-">, Group<f_Group>;
>  def ftemplate_backtrace_limit_EQ : Joined<["-"],
> "ftemplate-backtrace-limit=">,
>                                     Group<f_Group>;
> +def foperator_arrow_depth_EQ : Joined<["-"], "foperator-arrow-depth=">,
> +                               Group<f_Group>;
>  def ftest_coverage : Flag<["-"], "ftest-coverage">, Group<f_Group>;
>  def fvectorize : Flag<["-"], "fvectorize">, Group<f_Group>,
>    HelpText<"Enable the loop vectorization passes">;
>
>
> Attaching the gcc testsuite test case too for reference.
>

Please don't mail code from GCC here.

Instead, you'll need to craft a test case for our test suite. You can take
inspiration from

  test/SemaCXX/constexpr-depth.cpp
  test/SemaTemplate/instantiation-depth.cpp


> How do I add the flag to the documentation?
>

Modify docs/UsersManual.rst

Thank you for working on this!


> Thanks,
> Rahul
>
>
> On Sat, Oct 26, 2013 at 12:14 AM, Richard Smith <richard at metafoo.co.uk>wrote:
>
>> On Fri, Oct 25, 2013 at 9:06 AM, Arthur O'Dwyer <
>> arthur.j.odwyer at gmail.com> wrote:
>>
>>> IMHO, it would be better to remove the command-line option, and just
>>> set the (hard-coded) limit to 2048 or something else of that general
>>> order of magnitude. I can't think of any valid use for deeply nested
>>> operator->s; it's not like recursive template instantiation, where you
>>> might want to use a higher limit to implement a useful behavior.
>>>
>>
>> The "usual" (albeit very rare) use is to get around the recursive
>> template instantiation depth limit.
>>
>>
>>> Therefore, setting the limit to something in the thousands ought to be
>>> good enough until someone legitimately runs into the limit; and in the
>>> meantime, we're not polluting the command-line docs with obscure and
>>> useless options.
>>>
>>
>> I don't see any problem with setting the default to a higher value, like
>> maybe 2048.
>>
>>
>>> Also FWIW, I do think a DR ought to be opened to make this an explicit
>>> implementation limit in the Standard... but I'm not volunteering. :)
>>>
>>> my $.02,
>>> –Arthur
>>>
>>>
>>> On Fri, Oct 25, 2013 at 7:45 AM, Rahul Jain <1989.rahuljain at gmail.com>
>>> wrote:
>>> >
>>> > Hi,
>>> >
>>> > This is with reference to the below discussion thread:
>>> >
>>> >
>>> http://clang-developers.42468.n3.nabble.com/Endless-operator-gt-chain-causing-infinite-loop-td4035258.html
>>> >
>>> > I have tried to implement a command line argument to handle the issue.
>>> >
>>> > The patch for the same is below:
>>> >
>>> > Index: lib/Driver/Tools.cpp
>>> > ===================================================================
>>> > --- lib/Driver/Tools.cpp    (revision 193400)
>>> > +++ lib/Driver/Tools.cpp    (working copy)
>>> > @@ -2802,6 +2802,12 @@
>>> >      CmdArgs.push_back(A->getValue());
>>> >    }
>>> >
>>> > +  if (Arg *A = Args.getLastArg(options::OPT_foperatorarrow_depth_,
>>> > +                               options::OPT_foperatorarrow_depth_EQ))
>>> {
>>> > +    CmdArgs.push_back("-foperatorarrow-depth");
>>>
>>
>> Please use foperator_arrow_depth and -foperator-arrow-depth.
>>
>>
>>>  > +    CmdArgs.push_back(A->getValue());
>>> > +  }
>>> > +
>>> >    if (Arg *A = Args.getLastArg(options::OPT_fconstexpr_depth_EQ)) {
>>> >      CmdArgs.push_back("-fconstexpr-depth");
>>> >      CmdArgs.push_back(A->getValue());
>>> > Index: lib/Frontend/CompilerInvocation.cpp
>>> > ===================================================================
>>> > --- lib/Frontend/CompilerInvocation.cpp    (revision 193400)
>>> > +++ lib/Frontend/CompilerInvocation.cpp    (working copy)
>>> > @@ -1313,6 +1313,8 @@
>>> >    Opts.MathErrno = !Opts.OpenCL && Args.hasArg(OPT_fmath_errno);
>>> >    Opts.InstantiationDepth =
>>> >        getLastArgIntValue(Args, OPT_ftemplate_depth, 256, Diags);
>>> > +  Opts.ArrowDepth =
>>> > +      getLastArgIntValue(Args, OPT_foperatorarrow_depth, 256, Diags);
>>> >    Opts.ConstexprCallDepth =
>>> >        getLastArgIntValue(Args, OPT_fconstexpr_depth, 512, Diags);
>>> >    Opts.ConstexprStepLimit =
>>> > Index: lib/Sema/SemaExprCXX.cpp
>>> > ===================================================================
>>> > --- lib/Sema/SemaExprCXX.cpp    (revision 193400)
>>> > +++ lib/Sema/SemaExprCXX.cpp    (working copy)
>>> > @@ -5188,8 +5188,19 @@
>>> >      llvm::SmallPtrSet<CanQualType,8> CTypes;
>>> >      SmallVector<SourceLocation, 8> Locations;
>>> >      CTypes.insert(Context.getCanonicalType(BaseType));
>>> > +
>>> > +    static unsigned int arrow_instantiation_count = 0;
>>>
>>
>> This should not be static, and the name should follow the coding
>> convention:
>>
>>
>> http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
>>
>> > +    while (BaseType->isRecordType()) {
>>> > +      arrow_instantiation_count++;
>>> > +      if (arrow_instantiation_count >= getLangOpts().ArrowDepth) {
>>> > +        Diag(OpLoc,diag::err_arrow_instantiation_depth_exceeded)
>>>
>>
>> Space after comma.
>>
>>
>>>  > +        << getLangOpts().ArrowDepth
>>> > +        << Base->getSourceRange();
>>> > +        Diag(OpLoc, diag::note_arrow_instantiation_depth)
>>> > +        << getLangOpts().ArrowDepth;
>>> > +    return ExprError();
>>>
>>
>> Indentation is strange here.
>>
>>
>>>  > +      }
>>> >
>>> > -    while (BaseType->isRecordType()) {
>>> >        Result = BuildOverloadedArrowExpr(
>>> >            S, Base, OpLoc,
>>> >            // When in a template specialization and on the first loop
>>> > iteration,
>>> > Index: include/clang/Basic/DiagnosticSemaKinds.td
>>> > ===================================================================
>>> > --- include/clang/Basic/DiagnosticSemaKinds.td    (revision 193400)
>>> > +++ include/clang/Basic/DiagnosticSemaKinds.td    (working copy)
>>> > @@ -2561,6 +2561,11 @@
>>> >  // can handle that case properly.
>>> >  def note_ovl_candidate_non_deduced_mismatch_qualified : Note<
>>> >      "candidate template ignored: could not match %q0 against %q1">;
>>> > +def err_arrow_instantiation_depth_exceeded : Error<
>>> > +  "arrow operator instantiation exceeded maximum depth of %0">,
>>> > +  DefaultFatal, NoSFINAE;
>>>
>>
>> We don't need this to be fatal.
>>
>>
>>>  > +def note_arrow_instantiation_depth : Note<
>>> > +  "use -foperatorarrow-depth=N to increase arrow operator
>>> instantiation
>>> > depth">;
>>> >
>>> >  // Note that we don't treat templates differently for this diagnostic.
>>> >  def note_ovl_candidate_arity : Note<"candidate "
>>> > Index: include/clang/Basic/LangOptions.def
>>> > ===================================================================
>>> > --- include/clang/Basic/LangOptions.def    (revision 193400)
>>> > +++ include/clang/Basic/LangOptions.def    (working copy)
>>> > @@ -160,6 +160,8 @@
>>> >  ENUM_LANGOPT(SignedOverflowBehavior, SignedOverflowBehaviorTy, 2,
>>> > SOB_Undefined,
>>> >               "signed integer overflow handling")
>>> >
>>> > +BENIGN_LANGOPT(ArrowDepth, 32, 256,
>>> > +               "maximum arrow instantiation depth")
>>>
>>
>> "instantiation" is not correct here. Maybe "maximum number of operator->s
>> to follow"?
>>
>>
>>> >  BENIGN_LANGOPT(InstantiationDepth, 32, 256,
>>> >                 "maximum template instantiation depth")
>>> >  BENIGN_LANGOPT(ConstexprCallDepth, 32, 512,
>>> > Index: include/clang/Driver/CC1Options.td
>>> > ===================================================================
>>> > --- include/clang/Driver/CC1Options.td    (revision 193400)
>>> > +++ include/clang/Driver/CC1Options.td    (working copy)
>>> > @@ -442,6 +442,8 @@
>>> >    HelpText<"Default type visibility">;
>>> >  def ftemplate_depth : Separate<["-"], "ftemplate-depth">,
>>> >    HelpText<"Maximum depth of recursive template instantiation">;
>>> > +def foperatorarrow_depth : Separate<["-"], "foperatorarrow-depth">,
>>> > +  HelpText<"Maximum depth of arrow operator instantiation">;
>>> >  def fconstexpr_depth : Separate<["-"], "fconstexpr-depth">,
>>> >    HelpText<"Maximum depth of recursive constexpr function calls">;
>>> >  def fconstexpr_steps : Separate<["-"], "fconstexpr-steps">,
>>> > Index: include/clang/Driver/Options.td
>>> > ===================================================================
>>> > --- include/clang/Driver/Options.td    (revision 193400)
>>> > +++ include/clang/Driver/Options.td    (working copy)
>>> > @@ -775,6 +775,8 @@
>>> >  def ftemplate_depth_ : Joined<["-"], "ftemplate-depth-">,
>>> Group<f_Group>;
>>> >  def ftemplate_backtrace_limit_EQ : Joined<["-"],
>>> > "ftemplate-backtrace-limit=">,
>>> >                                     Group<f_Group>;
>>> > +def foperatorarrow_depth_EQ : Joined<["-"], "foperatorarrow-depth=">,
>>> > Group<f_Group>;
>>> > +def foperatorarrow_depth_ : Joined<["-"], "foperatorarrow-depth-">,
>>> > Group<f_Group>;
>>>
>>
>> Please remove this variant. We only support the - form for
>> -ftemplate-depth for compatibility with an old g++ misstep.
>>
>>
>>>  >  def ftest_coverage : Flag<["-"], "ftest-coverage">, Group<f_Group>;
>>> >  def fvectorize : Flag<["-"], "fvectorize">, Group<f_Group>,
>>> >    HelpText<"Enable the loop vectorization passes">;
>>> >
>>> >
>>> >
>>> > Please if someone could help me by reviewing it and suggesting
>>> improvements?
>>>
>>
>> It looks basically right, other than the tweaks above. Thanks!
>>
>> Please also add this flag to the documentation.
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131028/a5ed3b42/attachment.html>