[flang] Stricter "implicit continuation" in preprocessing

[klausler]

Summary

The prescanner performs implicit line continuation when it looks
like the parenthesized arguments of a call to a function-like macro
may span multiple lines. In an attempt to work more like a
Fortran-oblivious C preprocessor, the prescanner will act as if
the following lines had been continuations so that the function-like
macro could be invoked.

This still seems like a good idea, but a recent bug report on
LLVM's GitHub issue tracker shows one way in which it could trigger
inadvertently and mess up a program. So this patch makes the
conditions for implicit line continuation much more strict.

First, the leading parenthesis has to have been preceded by an
identifier that's known to be a macro name. (It doesn't have to
be a function-like macro, since it's possible for a keyword-like
macro to expand to the name of a function-like macro.) Second,
no macro definition can ever have had unbalanced parentheses in
its replacement text.

Also cleans up some parenthesis recognition code to fix some
issues found in testing, so that a token with leading or trailing
spaces can still be recognized as a parenthesis or comma.

Fixes https://github.com/llvm/llvm-project/issues/63844.

[rogfer01]

Hi @klausler can you upload a patch with context. I left a few initial comments though.

Thanks!

flang/docs/Preprocessing.md

77

I'm not sure there is a test already for this case, is there?

Just to confirm I understand this correctly, this new paragraph covers this case below:

subroutine sub(a)
  implicit none
  integer :: a

#define foo(x) ((x) + 1)
#define bar foo

  a = bar  (
a
)

end subroutine sub

that should be preprocessed something similar to:

subroutine sub(a)
  implicit none
  integer :: a

  a = (( a ) + 1)

end subroutine sub

flang/include/flang/Parser/char-block.h

67

I'm confused by this function. I've tested it with several inputs and it always seems to return ' '?

My tests here: https://www.godbolt.org/z/af9oc74aE

Maybe I'm missing something. Can you add a comment about its purpose?

[klausler]

flang/docs/Preprocessing.md

77

Yes.

flang/include/flang/Parser/char-block.h

67

It returns the only non-blank character, if it is the only non-blank character.

[klausler]

Add comment.

[rogfer01]

Thanks for the update, @klausler

I added some more comments.

(Pretty please 🙂 , add the full context otherwise reviewing the change gets unnecessarily harder)

flang/include/flang/Parser/char-block.h

67

Ah, right a case I forgot to test 😅 . Thanks.

Would it make sense not to use a loop? Something like this:

char OnlyNonBlank() const {
    if (size() == 1) {
        char ch{*begin()};
        if (ch != ' ' && ch != '\t') {
            return ch;
        }
    }
    return ' ';
}

flang/lib/Parser/preprocessor.cpp

1198

anyMacroWithUnbalancedParantheses_ is only getting updated when we find a directive and we do not seem to clean it up, so we end carrying wrong state up to the point where we decide we allow an implicit continuation.

Consider the following testcase:

program main
  implicit none
#define FOO(x) ((x) + 1)
#define BAR (
  integer :: k

  k = FOO(
    4)
end program main

This fails with the patch

$ flang-new -E -o- t.F90 
error: Could not scan t.F90
./t.F90:8:10: error: Unmatched '('
    k = FOO(
           ^
./t.F90:9:6: error: Unmatched ')'
      4)
       ^

but if you remove (or comment) the definition of BAR the file is scanned correctly.

flang-new -E -o- t.F90 
#line "./t.F90" 2
      program main
      implicit none


      integer :: k

      k = (( 4) + 1)

      end program main

[klausler]

flang/include/flang/Parser/char-block.h

67

The entire point of this function is to ignore leading and trailing blanks to expose a single-character token. Every character needs to be examined. This requires a loop.

flang/lib/Parser/preprocessor.cpp

1198

The whole point of this flag is to disable all implicit continuations if any macro definition has unbalanced parentheses. I can't figure out how to do it safely and I've spent too much time on this topic already.

[klausler]

Fix setting of anyMacroWithUnbalancedParentheses_ -- once true, it must stay true.

[klausler]

ping

[clementval]

LGTM

[klausler]

Any further comments, rogfer01?

[rogfer01]

LGTM. I suggest adding an XFAIL'd test but I'll leave this at your choice.

flang/include/flang/Parser/char-block.h

67

Right. Thanks for the clarification!

flang/lib/Parser/preprocessor.cpp

1198

Right. My concern is that the definition of a macro with unbalanced parentheses that it is not even used in the code causes a later implicit continuation to be disabled.

This is a bit surprising and I think it may be confusing. But it seems an unusual situation so I understand its impact is low and can be fixed later. I suggest to add a XFAIL testcase for it at least.

(gfortran and pgf90 don't have a problem with this. ifort/ifx rejects that but that seems because it does not support this kind of implicit continuations.)

flang/docs/Preprocessing.md

 
 * Expressions in `#if` and `#elif` should support both Fortran and C
   operators; e.g., `#if 2 .LT. 3` should work.
 * If a function-like macro does not close its parentheses, line
   continuation should be assumed.
+  This includes the case of a keyword-like macro that expands to
+  the name of a function-like macro.
 * ... However, the leading parenthesis has to be on the same line as
   the name of the function-like macro, or on a continuation line thereof.
+* And no macro definition prior to that point can be allowed to have
+  unbalanced parentheses in its replacement text.
+  When that happens, it's possible to have false positive cases
+  causing implicit line continuations that break real code.
 * If macros expand to text containing `&`, it doesn't work as a free form
   line continuation marker.
 * `#define c 1` does not allow a `c` in column 1 to be used as a label
   in fixed form, rather than as a comment line indicator.
 * IBM claims to be ISO C compliant and therefore recognizes trigraph sequences.

 . . . . . .   pp122.F90  KWM NOT expanded in "literal"
 . . E E . E   pp123.F90  KWM NOT expanded in Hollerith literal
 . . E E . E   pp124.F90  KWM NOT expanded in Hollerith in FORMAT
 E . . E E .   pp125.F90  #DEFINE works in free form
 . . . . . .   pp126.F90  \ newline works in #define
-N . E . E E   pp127.F90  FLM call with closing ')' on next line (not a continuation)
+. . E . E E   pp127.F90  FLM call with closing ')' on next line (not a continuation)
 E . E . E E   pp128.F90  FLM call with '(' on next line (not a continuation)
 . . N . . N   pp129.F90  #define KWM !, then KWM works as comment line initiator
 E . E . . E   pp130.F90  #define KWM &, use for continuation w/o pasting (ifort and nag seem to continue #define)
 ```

flang/include/flang/Parser/char-block.h

       }
     }
     return ' '; // non no-blank character
   }
 
+  char OnlyNonBlank() const {
+    char result{' '};
+    for (char ch : *this) {
+      if (ch != ' ' && ch != '\t') {
+        if (result == ' ') {
+          result = ch;
+        } else {
+          return ' ';
+        }
+      }
+    }
+    return result;
+  }
+
   std::size_t CountLeadingBlanks() const {
     std::size_t n{size()};
     std::size_t j{0};
     for (; j < n; ++j) {
       char ch{(*this)[j]};

flang/lib/Parser/preprocessor.h

       const TokenSequence &, Prescanner &);
 
   // Implements a preprocessor directive.
   void Directive(const TokenSequence &, Prescanner &);
 
+  bool anyMacroWithUnbalancedParentheses() const {
+    return anyMacroWithUnbalancedParentheses_;
+  }
+
 private:
   enum class IsElseActive { No, Yes };
   enum class CanDeadElseAppear { No, Yes };
 
   CharBlock SaveTokenAsName(const CharBlock &);

   void SkipDisabledConditionalCode(
       const std::string &, IsElseActive, Prescanner &, ProvenanceRange);
   bool IsIfPredicateTrue(const TokenSequence &expr, std::size_t first,
       std::size_t exprTokens, Prescanner &);
   void LineDirective(const TokenSequence &, std::size_t, Prescanner &);
+  void CheckForUnbalancedParentheses(
+      const TokenSequence &, std::size_t first, std::size_t tokens);
 
   AllSources &allSources_;
   std::list<std::string> names_;
   std::unordered_map<CharBlock, Definition> definitions_;
   std::stack<CanDeadElseAppear> ifStack_;
+  bool anyMacroWithUnbalancedParentheses_{false};
 };
 } // namespace Fortran::parser
 #endif // FORTRAN_PARSER_PREPROCESSOR_H_

flang/lib/Parser/preprocessor.cpp

   std::size_t tokens{replacement_.SizeInTokens()};
   for (std::size_t j{0}; j < tokens; ++j) {
     CharBlock token{replacement_.TokenAt(j)};
     std::size_t bytes{token.size()};
     if (skipping) {
-      if (bytes == 1) {
-        if (token[0] == '(') {
-          ++parenthesesNesting;
-        } else if (token[0] == ')') {
-          skipping = --parenthesesNesting > 0;
+      char ch{token.OnlyNonBlank()};
+      if (ch == '(') {
+      } else if (ch == ')') {
+        if (parenthesesNesting > 0) {
+          --parenthesesNesting;
         }
+        skipping = parenthesesNesting > 0;
       }
       continue;
     }
     if (bytes == 2 && token[0] == '~') { // argument substitution
       std::size_t index = token[1] - 'A';

           result.Put(","s, commaProvenance);
         }
         result.Put(args[k]);
       }
     } else if (bytes == 10 && isVariadic_ && token.ToString() == "__VA_OPT__" &&
-        j + 2 < tokens && replacement_.TokenAt(j + 1).ToString() == "(" &&
+        j + 2 < tokens && replacement_.TokenAt(j + 1).OnlyNonBlank() == '(' &&
         parenthesesNesting == 0) {
       parenthesesNesting = 1;
       skipping = args.size() == argumentCount_;
       ++j;
     } else {
-      if (bytes == 1 && parenthesesNesting > 0 && token[0] == '(') {
-        ++parenthesesNesting;
-      } else if (bytes == 1 && parenthesesNesting > 0 && token[0] == ')') {
-        if (--parenthesesNesting == 0) {
-          skipping = false;
-          continue;
+      if (parenthesesNesting > 0) {
+        char ch{token.OnlyNonBlank()};
+        if (ch == '(') {
+          ++parenthesesNesting;
+        } else if (ch == ')') {
+          if (--parenthesesNesting == 0) {
+            skipping = false;
+            continue;
+          }
         }
       }
       result.Put(replacement_, j);
     }
   }

       continue;
     }
     std::vector<std::size_t> argStart{++k};
     for (int nesting{0}; k < tokens; ++k) {
       CharBlock token{input.TokenAt(k)};
-      if (token.size() == 1) {
-        char ch{token[0]};
-        if (ch == '(') {
-          ++nesting;
-        } else if (ch == ')') {
-          if (nesting == 0) {
-            break;
-          }
-          --nesting;
-        } else if (ch == ',' && nesting == 0) {
-          argStart.push_back(k + 1);
+      char ch{token.OnlyNonBlank()};
+      if (ch == '(') {
+        ++nesting;
+      } else if (ch == ')') {
+        if (nesting == 0) {
+          break;
         }
+        --nesting;
+      } else if (ch == ',' && nesting == 0) {
+        argStart.push_back(k + 1);
       }
     }
     if (argStart.size() == 1 && k == argStart[0] && def->argumentCount() == 0) {
       // Subtle: () is zero arguments, not one empty argument,
       // unless one argument was expected.

           "#define: missing or invalid name"_err_en_US);
       return;
     }
     nameToken = SaveTokenAsName(nameToken);
     definitions_.erase(nameToken);
-    if (++j < tokens && dir.TokenAt(j).size() == 1 &&
-        dir.TokenAt(j)[0] == '(') {
+    if (++j < tokens && dir.TokenAt(j).OnlyNonBlank() == '(') {
       j = dir.SkipBlanks(j + 1);
       std::vector<std::string> argName;
       bool isVariadic{false};
-      if (dir.TokenAt(j).ToString() != ")") {
+      if (dir.TokenAt(j).OnlyNonBlank() != ')') {
         while (true) {
           std::string an{dir.TokenAt(j).ToString()};
           if (an == "...") {
             isVariadic = true;
           } else {

           if (j == tokens) {
             prescanner.Say(dir.GetTokenProvenanceRange(tokens - 1),
                 "#define: malformed argument list"_err_en_US);
             return;
           }
-          std::string punc{dir.TokenAt(j).ToString()};
-          if (punc == ")") {
+          char punc{dir.TokenAt(j).OnlyNonBlank()};
+          if (punc == ')') {
             break;
           }
-          if (isVariadic || punc != ",") {
+          if (isVariadic || punc != ',') {
             prescanner.Say(dir.GetTokenProvenanceRange(j),
                 "#define: malformed argument list"_err_en_US);
             return;
           }
           j = dir.SkipBlanks(j + 1);

               "#define: argument names are not distinct"_err_en_US);
           return;
         }
       }
       j = dir.SkipBlanks(j + 1);
+      CheckForUnbalancedParentheses(dir, j, tokens - j);
       definitions_.emplace(std::make_pair(
           nameToken, Definition{argName, dir, j, tokens - j, isVariadic}));
     } else {
       j = dir.SkipBlanks(j + 1);
+      CheckForUnbalancedParentheses(dir, j, tokens - j);
       definitions_.emplace(
           std::make_pair(nameToken, Definition{dir, j, tokens - j}));
     }
   } else if (dirName == "undef") {
     if (nameToken.empty()) {

     if (*error) {
       return 0;
     }
     switch (op) {
     case PARENS:
-      if (*atToken < tokens && token.TokenAt(*atToken).ToString() == ")") {
+      if (*atToken < tokens && token.TokenAt(*atToken).OnlyNonBlank() == ')') {
         ++*atToken;
         break;
       }
       if (*atToken >= tokens) {
         *error = Message{token.GetProvenanceRange(),

   TokenSequence expr2;
   for (std::size_t j{0}; j < expr1.SizeInTokens(); ++j) {
     if (ToLowerCaseLetters(expr1.TokenAt(j).ToString()) == "defined") {
       CharBlock name;
       if (j + 3 < expr1.SizeInTokens() &&
-          expr1.TokenAt(j + 1).ToString() == "(" &&
-          expr1.TokenAt(j + 3).ToString() == ")") {
+          expr1.TokenAt(j + 1).OnlyNonBlank() == '(' &&
+          expr1.TokenAt(j + 3).OnlyNonBlank() == ')') {
         name = expr1.TokenAt(j + 2);
         j += 3;
       } else if (j + 1 < expr1.SizeInTokens() &&
           IsLegalIdentifierStart(expr1.TokenAt(j + 1))) {
         name = expr1.TokenAt(++j);

       linePath = &*pos->path;
     }
     sourceFile->LineDirective(pos->trueLineNumber + 1, *linePath, *lineNumber);
   }
 }
+
+void Preprocessor::CheckForUnbalancedParentheses(
+    const TokenSequence &tokens, std::size_t j, std::size_t n) {
+  if (!anyMacroWithUnbalancedParentheses_) {
+    int nesting{0};
+    for (; n-- > 0; ++j) {
+      char ch{tokens.TokenAt(j).OnlyNonBlank()};
+      if (ch == '(') {
+        ++nesting;
+      } else if (ch == ')') {
+        if (nesting-- == 0) {
+          break;
+        }
+      }
+    }
+    anyMacroWithUnbalancedParentheses_ = nesting != 0;
+  }
+}
 } // namespace Fortran::parser

flang/lib/Parser/prescan.h

 
   void BeginStatementAndAdvance() {
     BeginSourceLineAndAdvance();
     slashInCurrentStatement_ = false;
     preventHollerith_ = false;
-    delimiterNesting_ = 0;
+    parenthesisNesting_ = 0;
     continuationLines_ = 0;
+    isPossibleMacroCall_ = false;
   }
 
   Provenance GetProvenance(const char *sourceChar) const {
     return startProvenance_ + (sourceChar - start_);
   }

   AllSources &allSources_;
   common::LanguageFeatureControl features_;
   bool inFixedForm_{false};
   int fixedFormColumnLimit_{72};
   Encoding encoding_{Encoding::UTF_8};
-  int delimiterNesting_{0};
+  int parenthesisNesting_{0};
   int prescannerNesting_{0};
   int continuationLines_{0};
+  bool isPossibleMacroCall_{false};
 
   Provenance startProvenance_;
   const char *start_{nullptr}; // beginning of current source file content
   const char *limit_{nullptr}; // first address after end of current source
   const char *nextLine_{nullptr}; // next line to process; <= limit_

flang/lib/Parser/prescan.cpp

       //  DATAC/N*2H  /    is repeated Hollerith
       preventHollerith_ = !slashInCurrentStatement_;
     }
   } else {
     char ch{*at_};
-    if (ch == '(' || ch == '[') {
-      ++delimiterNesting_;
-    } else if ((ch == ')' || ch == ']') && delimiterNesting_ > 0) {
-      --delimiterNesting_;
+    if (ch == '(') {
+      if (parenthesisNesting_++ == 0) {
+        isPossibleMacroCall_ = tokens.SizeInTokens() > 0 &&
+            preprocessor_.IsNameDefined(
+                tokens.TokenAt(tokens.SizeInTokens() - 1));
+      }
+    } else if (ch == ')' && parenthesisNesting_ > 0) {
+      --parenthesisNesting_;
     }
     char nch{EmitCharAndAdvance(tokens, ch)};
     preventHollerith_ = false;
     if ((nch == '=' &&
             (ch == '<' || ch == '>' || ch == '/' || ch == '=' || ch == '!')) ||

 }
 
 // Implicit line continuation allows a preprocessor macro call with
 // arguments to span multiple lines.
 bool Prescanner::IsImplicitContinuation() const {
-  return !inPreprocessorDirective_ && !inCharLiteral_ &&
-      delimiterNesting_ > 0 && !IsAtEnd() &&
+  return !inPreprocessorDirective_ && !inCharLiteral_ && isPossibleMacroCall_ &&
+      parenthesisNesting_ > 0 &&
+      !preprocessor_.anyMacroWithUnbalancedParentheses() && !IsAtEnd() &&
       ClassifyLine(nextLine_).kind == LineClassification::Kind::Source;
 }
 
 bool Prescanner::Continuation(bool mightNeedFixedFormSpace) {
   if (*at_ == '\n' || *at_ == '&') {

flang/lib/Parser/token-sequence.cpp

   // First, a quick pass with no allocation for the common case
   int nesting{0};
   std::size_t tokens{SizeInTokens()};
   for (std::size_t j{0}; j < tokens; ++j) {
     CharBlock token{TokenAt(j)};
-    char ch{token.FirstNonBlank()};
+    char ch{token.OnlyNonBlank()};
     if (ch == '(') {
       ++nesting;
     } else if (ch == ')') {
-      --nesting;
+      if (nesting-- == 0) {
+        break;
+      }
     }
   }
   if (nesting != 0) {
     // There's an error; diagnose it
     std::vector<std::size_t> stack;
     for (std::size_t j{0}; j < tokens; ++j) {
       CharBlock token{TokenAt(j)};
-      char ch{token.FirstNonBlank()};
+      char ch{token.OnlyNonBlank()};
       if (ch == '(') {
         stack.push_back(j);
       } else if (ch == ')') {
         if (stack.empty()) {
           messages.Say(GetTokenProvenanceRange(j), "Unmatched ')'"_err_en_US);

flang/test/Lower/array-elemental-calls-3.f90

   ! Nested elemental function calls.
   y = elem_func_r(cos(x))
   y = elem_func_r(cos(x) + u)
 
   ! Array constructors as elemental function arguments.
-  y = atan2( (/ (real(i, 4), i = 1, 2) /),
+  y = atan2( (/ (real(i, 4), i = 1, 2) /), &
              real( (/ (i, i = j, k, l) /), 4) )
 end subroutine
 
 end module

flang/test/Preprocessing/implicit-contin1.F90

+! RUN: %flang -E %s | FileCheck %s
+! When there's a macro definition with unbalanced parentheses,
+! don't apply implicit continuation.
+#define M )
+call foo (1 M
+end
+
+!CHECK:      call foo(1 )
+!CHECK:      end

flang/test/Preprocessing/implicit-contin2.F90

+! RUN: %flang -E %s | FileCheck %s
+! Test implicit continuation for possible function-like macro calls
+#define flm(x) x
+print *, flm(1)
+continue
+print *, flm(2
+)
+end
+
+!CHECK:      print *, 1
+!CHECK:      continue
+!CHECK:      print *, 2
+!CHECK:      end

flang/test/Preprocessing/implicit-contin3.F90

+! RUN: not %flang -E %s 2>&1 | FileCheck %s
+! Test implicit continuation for possible function-like macro calls only
+#define flm(x) x
+call notamacro(3
+)
+!CHECK: error: Unmatched '('
+!CHECK: error: Unmatched ')'

flang/test/Preprocessing/pp127.F90

 ! RUN: %flang -E %s 2>&1 | FileCheck %s
-! CHECK: res = IFLM(666 )
+! CHECK: res = ((666)+111)
 ! FLM call with closing ')' on next line (not a continuation)
       integer function IFLM(x)
         integer :: x
         IFLM = x
       end function IFLM

flang/test/Semantics/array-constr-big.f90

           ), &
           ( &
             ( &
               !ERROR: Must be a constant value
               0_foo,ii=1,limit &
-            ),
+            ), &
             jj=kk,limit &
           ), &
         kk=1,limit &
       ) /), &
              (/ limit /) )