Endless operator-> chain causing infinite loop
Richard Smith
richard at metafoo.co.uk
Fri Oct 25 11:44:12 PDT 2013
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/20131025/59c03add/attachment.html>
More information about the cfe-commits
mailing list