[MC,ELF] Emit warning if a string constant contains newline char #98060

chestnykh · 2024-07-08T18:05:54Z

GAS emits warning about newline in the string constant so make the same behaviour.

llvmbot · 2024-07-08T18:06:28Z

@llvm/pr-subscribers-backend-arm
@llvm/pr-subscribers-backend-webassembly
@llvm/pr-subscribers-backend-amdgpu

@llvm/pr-subscribers-mc

Author: Dmitriy Chestnykh (chestnykh)

Changes

GAS emits warning about newline in the string constant so make the same behaviour.

Full diff: https://github.com/llvm/llvm-project/pull/98060.diff

2 Files Affected:

(modified) llvm/lib/MC/MCParser/AsmLexer.cpp (+6-1)
(added) llvm/test/MC/ELF/warn-newline-in-string-constant.s (+6)

diff --git a/llvm/lib/MC/MCParser/AsmLexer.cpp b/llvm/lib/MC/MCParser/AsmLexer.cpp
index e08404ae0ad92f..eda7aedd9f5d3d 100644
--- a/llvm/lib/MC/MCParser/AsmLexer.cpp
+++ b/llvm/lib/MC/MCParser/AsmLexer.cpp
@@ -18,6 +18,7 @@
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/MC/MCAsmInfo.h"
 #include "llvm/MC/MCParser/MCAsmLexer.h"
+#include "llvm/Support/raw_ostream.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/SMLoc.h"
 #include "llvm/Support/SaveAndRestore.h"
@@ -646,13 +647,17 @@ AsmToken AsmLexer::LexQuote() {
     return AsmToken(AsmToken::String, StringRef(TokStart, CurPtr - TokStart));
   }
 
-  // TODO: does gas allow multiline string constants?
+  // gas doesn't allow multiline string constants
+  // and emits a warning if a string constant contains newline character.
   while (CurChar != '"') {
     if (CurChar == '\\') {
       // Allow \", etc.
       CurChar = getNextChar();
     }
 
+    if (CurChar == '\n')
+      outs() << "Warning: unterminated string; newline inserted\n";
+
     if (CurChar == EOF)
       return ReturnError(TokStart, "unterminated string constant");
 
diff --git a/llvm/test/MC/ELF/warn-newline-in-string-constant.s b/llvm/test/MC/ELF/warn-newline-in-string-constant.s
new file mode 100644
index 00000000000000..e126db30ee47a4
--- /dev/null
+++ b/llvm/test/MC/ELF/warn-newline-in-string-constant.s
@@ -0,0 +1,6 @@
+// RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux-gnu %s -o %t | FileCheck %s
+
+.string "abcdefg
+12345678"
+
+// CHECK: Warning: unterminated string; newline inserted

github-actions · 2024-07-08T18:09:37Z

✅ With the latest revision this PR passed the C/C++ code formatter.

s-barannikov

Printing text to stdout is not the correct way to report warnings, it should be done via source manager (and it is likely there are helper functions).
AsmLexer is a lexer, not parser. It does not know the context the lexed string is used. In some contexts the warning may not be desired. The warning, if needed, should be emitted by a parser that knows full context, e.g. in AsmParser::parseDirectiveAscii.

chestnykh · 2024-07-09T09:39:28Z

Printing text to stdout is not the correct way to report warnings, it should be done via source manager (and it is likely there are helper functions).

AsmLexer is a lexer, not parser. It does not know the context the lexed string is used. In some contexts the warning may not be desired. The warning, if needed, should be emitted by a parser that knows full context, e.g. in AsmParser::parseDirectiveAscii.

I reworked

llvm/lib/MC/MCParser/AsmLexer.cpp

llvm/test/MC/ELF/warn-newline-in-string-constant.s

llvm/lib/MC/MCParser/AsmParser.cpp

llvm/lib/MC/MCParser/DarwinAsmParser.cpp

llvm/test/MC/ELF/warn-newline-in-string-constant.s

llvm/lib/MC/MCParser/AsmParser.cpp

llvm/test/MC/ELF/warn-newline-in-string-constant.s

MaskRay · 2024-07-10T01:28:35Z

llvm/lib/MC/MCParser/AsmParser.cpp

@@ -3125,6 +3130,7 @@ bool AsmParser::parseDirectiveAscii(StringRef IDVal, bool ZeroTerminated) {
    do {
      if (parseEscapedString(Data))
        return true;
+


revert unneeded change

MaskRay · 2024-07-10T01:31:51Z

llvm/test/MC/ELF/warn-newline-in-escaped-string.s

+
+"
+
+// CHECK-WARN:   warn-newline-in-escaped-string.s:3:21: warning: unterminated string; newline inserted


Instead of string/string/string check/check/check, consider check/string check/string check/string that new tests use more.

You can use the following to test relative line numbers, so that inserting lines in the future would not require test update. We often omit the filename. When the filename is useful, use FileCheck %s -DFILE=%s

[[#@LINE+1]]:21: warning:

Thx, reworked

MaskRay · 2024-07-10T05:55:14Z

llvm/test/MC/ELF/warn-newline-in-escaped-string.s

+// CHECK-WARN:   [[#@LINE-8]]:28: warning: unterminated string; newline inserted
+// CHECK-WARN:   .asciz "another test string
+// CHECK-WARN:   [[#@LINE-9]]:1: warning: unterminated string; newline inserted
+// CHECK-WARN:   ^


You can delete ^ checks. With column numbers on the above line, ^ check isn't useful.

Use -NEXT whenever applicable

MaskRay · 2024-07-10T05:55:38Z

llvm/test/MC/ELF/warn-newline-in-escaped-string.s

+// CHECK-WARN:   [[#@LINE-2]]:20: warning: unterminated string; newline inserted
+
+.ascii "test\nstring\xFF\n\n\xFF"
+// CHECK-WARN-NOT:    [[#@LINE-1]]{{.*}}


You can use FileCheck --implicit-check-not=warning: and remove these -NOT: patterns

MaskRay · 2024-07-15T05:44:40Z

llvm/test/MC/ELF/warn-newline-in-escaped-string.s

@@ -0,0 +1,50 @@
+// RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux-gnu %s 2>&1 -o /dev/null | FileCheck -DFILE=%s --strict-whitespace %s --implicit-check-not=valid1_string --implicit-check-not=valid2_string --implicit-check-not=valid3_string --check-prefix=CHECK-WARN


Just use x86_64 as the triple to emphasize that the behavior is for all ELF (actually other object file formats as well, but we don't bother testing), not just Linux.

Done. I don't have write access so please merge this PR when it is ready

llvm/test/MC/ELF/warn-newline-in-escaped-string.s

.

s-barannikov

LGTM

GAS emits warning about newline in the string constant so make the same behaviour.

Emit warning about newline characters in strings for `.string`, '.ascii' and '.asciz' directives like GAS.

Move the point of check into `parseEscapedString`. Check if the `Warning()` returned true to stop compilation.

- Remove `WarnNewline` arg - Emit warning to `\n` symbol unconditionally

- Add new testcases

- Revert unneeded change - Refactor test to use string/check style and [[#@line]] instead of hardcoded line numbers

- Use `CHECK-NEXT` - Use `implicit-check-not` instead of `CHECK-WARN-NOT`

- Add one `--implicit-check-not` with the appropriate pattern - Refactor checks

- Remove redundant `--check-prefix` - Remove `-DFILE=`

MaskRay · 2024-07-15T20:16:54Z

llvm/test/MC/ELF/warn-newline-in-escaped-string.s

@@ -0,0 +1,55 @@
+// RUN: llvm-mc -filetype=obj -triple x86_64 %s 2>&1 -o /dev/null \
+// RUN: | FileCheck %s --implicit-check-not="{{[0-9]+:[0-9]+: warning: unterminated string}}"


We usually just test --implicit-check-not=warning:

MaskRay · 2024-07-15T20:17:38Z

subject

[MC,ELF] Emit warning if a string constant contains newline char.

The convention is to omit the trailing period in the subject

s-barannikov · 2024-07-15T20:52:40Z

I've rebased the branch to check if that would resolve CI failure (it looked unrelated).

MaskRay · 2024-07-16T03:42:02Z

llvm/test/MC/ELF/warn-newline-in-escaped-string.s

@@ -0,0 +1,55 @@
+// RUN: llvm-mc -filetype=obj -triple x86_64 %s 2>&1 -o /dev/null \
+// RUN: | FileCheck %s --implicit-check-not=warning:


continuation lines should be indented by 2

) Summary: GAS emits warning about newline in the string constant so make the same behaviour. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60250926

…112995) #98060 introduced a warning for unterminated string constants, however it was only checking for `\n` which means that it produced strange results on Windows (always blaming column 1) including having the [associated test fail](https://buildkite.com/llvm-project/github-pull-requests/builds/111277#0192a122-c5c9-4e4e-bc5b-7532fec99ae4) if Git happened to use Windows newlines when creating the file. This fix for this is to detect both `\r` and `\n`, but don't double-warn for Windows newlines.

llvmbot added the mc Machine (object) code label Jul 8, 2024

chestnykh force-pushed the warn-newline-in-string-constant branch 2 times, most recently from 58dfd35 to af41795 Compare July 8, 2024 18:11

chestnykh changed the title ~~Emit warning if a string constant contains newline char.~~ [MC,ELF] Emit warning if a string constant contains newline char. Jul 8, 2024

s-barannikov previously requested changes Jul 9, 2024

View reviewed changes

s-barannikov reviewed Jul 9, 2024

View reviewed changes

llvm/lib/MC/MCParser/AsmLexer.cpp Outdated Show resolved Hide resolved

llvm/test/MC/ELF/warn-newline-in-string-constant.s Outdated Show resolved Hide resolved

llvm/lib/MC/MCParser/AsmParser.cpp Outdated Show resolved Hide resolved

s-barannikov requested a review from MaskRay July 9, 2024 10:38

s-barannikov reviewed Jul 9, 2024

View reviewed changes

llvm/lib/MC/MCParser/AsmParser.cpp Outdated Show resolved Hide resolved

llvmbot added backend:ARM backend:AMDGPU backend:WebAssembly labels Jul 9, 2024

s-barannikov reviewed Jul 9, 2024

View reviewed changes

s-barannikov self-requested a review July 9, 2024 14:21

MaskRay reviewed Jul 10, 2024

View reviewed changes

chestnykh requested a review from MaskRay July 15, 2024 04:16

MaskRay reviewed Jul 15, 2024

View reviewed changes

MaskRay approved these changes Jul 15, 2024

View reviewed changes

s-barannikov reviewed Jul 15, 2024

View reviewed changes

llvm/test/MC/ELF/warn-newline-in-escaped-string.s Outdated Show resolved Hide resolved

s-barannikov self-requested a review July 15, 2024 07:26

s-barannikov reviewed Jul 15, 2024

View reviewed changes

llvm/test/MC/ELF/warn-newline-in-escaped-string.s Show resolved Hide resolved

llvm/test/MC/ELF/warn-newline-in-escaped-string.s Outdated Show resolved Hide resolved

s-barannikov approved these changes Jul 15, 2024

View reviewed changes

chestnykh added 2 commits July 15, 2024 22:33

[MC,ELF] Emit warning if a string constant contains newline char.

5ba8182

GAS emits warning about newline in the string constant so make the same behaviour.

[MC,ELF] Move warning handling to parser code

9cbe1ce

Emit warning about newline characters in strings for `.string`, '.ascii' and '.asciz' directives like GAS.

chestnykh added 12 commits July 15, 2024 22:33

[MC,AsmParser] Warn about newlines in some cases

8cc91db

Move the point of check into `parseEscapedString`. Check if the `Warning()` returned true to stop compilation.

[MC,AMDGPU] Correct code format

02b6535

[MC,AsmParser] Rework parseEscapedString

602019f

- Remove `WarnNewline` arg - Emit warning to `\n` symbol unconditionally

[MC,AsmParser] Rework test to newline warnings.

deeed1d

- Add new testcases

[MC,AsmParser] Adjuct test to newline warnings

c2baa2c

[MC,AsmParser] Correct code style

b30d756

[MC,AsmParser] Refactor AsmParser.c and the test

b866eff

- Revert unneeded change - Refactor test to use string/check style and [[#@line]] instead of hardcoded line numbers

[MC,AsmParser] Remove unneeded filename from the test

3cbfa5f

[MC,AsmParser] Refactor test

91f2c7f

- Use `CHECK-NEXT` - Use `implicit-check-not` instead of `CHECK-WARN-NOT`

[MC] Use x86_64 as triple in test

b43c4fa

[MC, ELF] Refactor test

60392b7

- Add one `--implicit-check-not` with the appropriate pattern - Refactor checks

[MC,ELF] Refactor the test

2f517c3

- Remove redundant `--check-prefix` - Remove `-DFILE=`

s-barannikov force-pushed the warn-newline-in-string-constant branch from c584425 to 2f517c3 Compare July 15, 2024 19:33

MaskRay reviewed Jul 15, 2024

View reviewed changes

chestnykh changed the title ~~[MC,ELF] Emit warning if a string constant contains newline char.~~ [MC,ELF] Emit warning if a string constant contains newline char Jul 16, 2024

chestnykh added 2 commits July 16, 2024 05:34

[MC,ELF] Simplify --implicit-check-not argument

d8cd073

[MC,ELF] Add missed colon

f1264d4

MaskRay reviewed Jul 16, 2024

View reviewed changes

[MC,ELF] Add needed indent

de4772d

s-barannikov merged commit bed625b into llvm:main Jul 16, 2024
7 checks passed

dpaoliello mentioned this pull request Oct 18, 2024

[MC] Fix llvm-mc unterminated string constants warning for Windows #112995

Merged


		"

		// CHECK-WARN: warn-newline-in-escaped-string.s:3:21: warning: unterminated string; newline inserted

		@@ -0,0 +1,50 @@
		// RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux-gnu %s 2>&1 -o /dev/null \| FileCheck -DFILE=%s --strict-whitespace %s --implicit-check-not=valid1_string --implicit-check-not=valid2_string --implicit-check-not=valid3_string --check-prefix=CHECK-WARN

		@@ -0,0 +1,55 @@
		// RUN: llvm-mc -filetype=obj -triple x86_64 %s 2>&1 -o /dev/null \
		// RUN: \| FileCheck %s --implicit-check-not="{{[0-9]+:[0-9]+: warning: unterminated string}}"

[MC,ELF] Emit warning if a string constant contains newline char #98060

[MC,ELF] Emit warning if a string constant contains newline char #98060

Uh oh!

Conversation

chestnykh commented Jul 8, 2024

Uh oh!

llvmbot commented Jul 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jul 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

s-barannikov left a comment

Choose a reason for hiding this comment

Uh oh!

chestnykh commented Jul 9, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaskRay Jul 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

s-barannikov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaskRay commented Jul 15, 2024

Uh oh!

s-barannikov commented Jul 15, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Jul 8, 2024 •

edited

Loading

github-actions bot commented Jul 8, 2024 •

edited

Loading

MaskRay Jul 10, 2024 •

edited

Loading