Endless operator-> chain causing infinite loop
Rahul Jain
1989.rahuljain at gmail.com
Sun Oct 27 14:10:52 PDT 2013
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;
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();
+ }
+
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">;
// 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.
How do I add the flag to the documentation?
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/f7b1f518/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: arrow1.C
Type: text/x-csrc
Size: 336 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131028/f7b1f518/attachment.c>
More information about the cfe-commits
mailing list