AsmParser: parse source comment using scanner instead of regex #15209

clonker · 2024-06-20T11:58:12Z

Fixes #15207
Fixes #13496

matheusaaguiar

Just some nitpicks and small suggestions.

libyul/AsmParser.cpp

matheusaaguiar · 2024-06-20T18:16:22Z

Also, just to confirm/point to, I see that we have some tests in test/libyul/Parser.cpp which seems to cover parsing. Not sure if we need or would benefit from adding something more there.

cameel

This needs a changelog entry for the bugfix.

Also note that github is a bit picky about the fixes/closes annotation. fixes issue #15207 did not work, The PR is not connected to the issue and won't close it automatically. You need Fixes #15207, i.e. without anything in between the word and the issue number.

Also, just to confirm/point to, I see that we have some tests in test/libyul/Parser.cpp which seems to cover parsing. Not sure if we need or would benefit from adding something more there.

The repro is a bit large and otherwise trivial, so I think it's ok to skip it.

I think we're missing some coverage for whitespace and newline handling behavior, because the PR passes all tests even though the way it handles whitespace seems to have changed.

libyul/AsmParser.cpp

Changelog.md

liblangutil/Scanner.cpp

libyul/AsmParser.cpp

liblangutil/Scanner.h

aarlt · 2024-06-25T16:01:08Z

I guess you could now also remove the rm ./*_bytecode_too_large_*.sol ./*_combined_too_large_*.sol in .circleci/parallel_bytecode_report.sh - if I remember correctly, the crashes where mainly happening because of the regexp stuff.

cameel · 2024-06-26T17:46:46Z

I guess you could now also remove the rm ./*_bytecode_too_large_*.sol ./*_combined_too_large_*.sol in .circleci/parallel_bytecode_report.sh - if I remember correctly, the crashes where mainly happening because of the regexp stuff.

Looks like it worked. Which is actually odd, because this PR doesn't fix #13496, does it? I mean, if it does then great, maybe we should close it too, but it's also possible that it got somehow fixed on develop and we didn't realize :)

clonker · 2024-06-27T09:21:35Z

I guess you could now also remove the rm ./*_bytecode_too_large_*.sol ./*_combined_too_large_*.sol in .circleci/parallel_bytecode_report.sh - if I remember correctly, the crashes where mainly happening because of the regexp stuff.

Looks like it worked. Which is actually odd, because this PR doesn't fix #13496, does it? I mean, if it does then great, maybe we should close it too, but it's also possible that it got somehow fixed on develop and we didn't realize :)

It does segfault for me in the issue you linked above on develop and not on this branch when, e.g., invoking solc --via-ir --asm test.sol. I assume it is because of the debug comments:

Invoking the same with --debug-info none does not segfault on develop, either.

aarlt · 2024-06-27T11:57:45Z

I guess you could now also remove the rm ./*_bytecode_too_large_*.sol ./*_combined_too_large_*.sol in .circleci/parallel_bytecode_report.sh - if I remember correctly, the crashes where mainly happening because of the regexp stuff.

Looks like it worked. Which is actually odd, because this PR doesn't fix #13496, does it? I mean, if it does then great, maybe we should close it too, but it's also possible that it got somehow fixed on develop and we didn't realize :)

It does segfault for me in the issue you linked above on develop and not on this branch when, e.g., invoking solc --via-ir --asm test.sol. I assume it is because of the debug comments:

Invoking the same with --debug-info none does not segfault on develop, either.

yep exactly! its because of the debug comments. I ran into this when I was working on the debug attribute parsing stuff. I would say we should merge this soon so I can rework my PR.

aarlt · 2024-06-27T12:00:20Z

In general it is a bug in std::regexp in some GCC versions that created that segfault.

Changelog.md

cameel

I think we still need some some adjustments in the scanner and a few more tests. Other than that it looks pretty good now.

cameel · 2024-06-27T16:38:50Z

liblangutil/Scanner.cpp

+ if (c == '\\')
+ scanEscape();


If I understand this code correctly, this does not quite match the behavior of the original regex. The code here will just skip invalid escape sequences and stray \ at the end of input. If we want to match the original regex, we'd have to instead:

not interpret unrecognized escapes, just keep them as is,

report an unterminated string if we reach \ at the end of input (well, I guess reporting an illegal escape instead would be fine too, as long as it's still an error).

Yeah it's true, it wasn't reflecting the RegEx behavior - although snippets are really just skipped during parsing, the important bit is the correct detection of the tail. That being said, it absolutely makes sense to interpret the 'special comment' literal string sensibly. Now everything that is escaped and interpretable is being interpreted as such and the rest is appended verbatim. I have added a couple tests to the scanner for that.

By the way, the most sensible behavior for such escapes would actually be to report an error, but that's breaking. They should never happen in Yul coming from the codegen and are only possible in user-supplied Yul, so maybe it's acceptable, buy I wouldn't do it in this PR. Maybe this is something that we should consider later though. Really, neither skipping them, nor keeping them "as is" is ideal.

libyul/AsmParser.cpp

test/libyul/Parser.cpp

cameel · 2024-06-27T19:02:26Z

It does segfault for me in the issue you linked above on develop and not on this branch when, e.g., invoking solc --via-ir --asm test.sol. I assume it is because of the debug comments:

Ah, right. I focused on parsing the long hex string (which should not be affected by this PR) but overlooked the fact that such a string will also end up in a snippet as a part of debug info. In that case, yeah, this PR does fix it.

cameel

This generally looks good so I'm approving already.

I still have some final suggestions, mostly regarding wording and comments. The only bigger one would to change how unterminated escapes are handled, though that's not incorrect, just leaves some unexpected artifacts in the (invalid) literal.

Changelog.md

test/libyul/Parser.cpp

liblangutil/Scanner.cpp

cameel · 2024-06-28T16:12:42Z

liblangutil/Scanner.cpp

+ if (c == '\\')
+ scanEscape();


It would be more consistent with the other branch to treat unterminated \ as IllegalEscapeSequence. Currently you continue, run scanEscape() and then error with IllegalStringEndQuote right after the loop.

It does work, but it's still weird to let scanEscape() run when we're at the end of input and m_char is 0, which I guess is assumed to never actually be read. The function will actually take that 0 and put into the literal and that literal is still accessible via currentLiteral() even when we error out.

Also, you should either handle the return value from scanEscape() or assert that it's not supposed to fail:

Suggested change

if (c == '\\')

scanEscape();

if (c == '\\')

{

if (isSourcePastEndOfInput())

return setError(ScannerError::IllegalEscapeSequence);

bool validEscape = scanEscape();

solAssert(validEscape);

}

Right! Done that, tests now reflect a case where there's a dangling backslash R"("test\)" and a missing string end quote R"("test)".

ekpyron · 2024-07-01T11:18:27Z

liblangutil/Scanner.cpp

+ {
+ if (isSourcePastEndOfInput())
+ return setError(ScannerError::IllegalEscapeSequence);
+ bool const validEscape = scanEscape(false /* _rejectInvalidEscapes */);


So just that I understand correctly: the only thing you need to do here is to check for escaped quotes, right? Couldn't you do that here directly instead of the entire refactoring of scanEscape? We don't actually do anything with the source quote in the snippets other than discarding them, so not sure why we'd go out of our way to deal with all sorts of escape sequences here...

The change was based off of this comment. I am completely fine with just checking for escaped quotes as well - all depends on the intended use of the scanner mode beyond this pr I guess.

scanEscape(false) is generally rather weird, though, isn't it? Either you want to properly handle all kinds of escape sequences or you don't, but handling some properly and ignoring others is strange, isn't it :-)?
I think the simpler solution to just only handle escaped quotes here matches more directly what we need now, and keeps the PR simpler - and if we ever want to use this for anything else we can revisit this then...

We don't even need to check for escaped quotes, this should be enough for the purpose of just skipping over it

if (c == '\\') { if (isSourcePastEndOfInput()) return setError(ScannerError::IllegalEscapeSequence); advance(); }

Yeah, and that's exactly what the regex did, right? Let's just do that then.

Was a code snippet runinng to EOF here actually even an error before the change :-)?

It would have reported an error because of missing end quote (eg "literal\ - the \ is no longer part of the match so the regex would have found "literal). Or did you mean in the current develop implementation? There as well

Well, when scanning string literals hitting EOF is an error. But I meant, whether it was an error if a source snippet of a comment in Yul, parsed with the regex, it was an error if the quote didn't terminate :-).
Technically, it it wasn't an error then, making it an error now is breaking.
There is error 1544_error even before this PR, but will this ever be hit in this case, i.e. will the regex even match that? Ah, the terminating quote there is "? right? So we do get that error in that case also on develop? Then it's fine.

Yeah, the previous condition for error 1544 is rather convoluted, but looks like that's what it did.

ekpyron · 2024-07-01T12:50:23Z

test/libyul/Parser.cpp

+ /// @src 1 : 111:
+ /// 222 "
+ /// abc\"def
+ ///
+ /// " @src 0:333:444


Where and how does this actually happen now - and how does it prevent

/// @src 1:1:1 " let x := 42 /// "

from swallowing the non-comment?

in the example it'll just skip all escaped stuff, so the snippet amounts to abcdef. the non-comment is not swallowed because it will never reach that part of the asmparser, there's an outer loop taking care of that, so only 1:1:1 " would arrive in parseSrcComment and error out with unterminated quote

so more precisely the input to parseSrcComment in the test you referenced up there is (with escapes)

1\t: 111:\n222 "\nabc\"def\n\n" @src 0:333:444

for which (1,111,222) is identified as components and abcdef as snippet (the rest is now skipped by scanner as discussed); everything else goes into the tail (being @src 0:333:444) which is returned to an outer loop.

The outer loop will match it as source location comment and parseSrcComment is called again, this time with 0:333:444 as input.

Ah, the point is that Scanner::scanSingleLineDocComment() just merges the contents of all consecutive lines starting with /// (but not more than that) - alright!

ekpyron

I'd say I'm reasonably convinced by this now and Kamil approved it earlier, so I'm fine with merging after squashing the commits a bit!

aarlt · 2024-07-01T18:46:45Z

Just to leave it here: the segmentation fault was related to a bug in GCC's std::regex implementation. This bug is at least reproducible with GCC 9, 10, 11, 12, 13 & 14.

clonker force-pushed the asm_parser_use_scanner branch from ff37008 to f3b731e Compare June 20, 2024 12:41

clonker marked this pull request as ready for review June 20, 2024 13:04

matheusaaguiar reviewed Jun 20, 2024

View reviewed changes

libyul/AsmParser.cpp Outdated Show resolved Hide resolved

libyul/AsmParser.cpp Outdated Show resolved Hide resolved

libyul/AsmParser.cpp Outdated Show resolved Hide resolved

libyul/AsmParser.cpp Outdated Show resolved Hide resolved

cameel added refactor and removed refactor labels Jun 20, 2024

cameel reviewed Jun 21, 2024

View reviewed changes

nikola-matic reviewed Jun 21, 2024

View reviewed changes

libyul/AsmParser.cpp Outdated Show resolved Hide resolved

clonker force-pushed the asm_parser_use_scanner branch 2 times, most recently from 7cf8e50 to b3fd10e Compare June 21, 2024 16:21

cameel reviewed Jun 21, 2024

View reviewed changes

clonker force-pushed the asm_parser_use_scanner branch from b3fd10e to 5bca035 Compare June 24, 2024 08:21

clonker mentioned this pull request Jun 24, 2024

Numerical Yul node id handles #15215

Draft

10 tasks

clonker force-pushed the asm_parser_use_scanner branch from 5bca035 to a1fb95f Compare June 26, 2024 14:06

aarlt reviewed Jun 27, 2024

View reviewed changes

Changelog.md Outdated Show resolved Hide resolved

clonker force-pushed the asm_parser_use_scanner branch from a1fb95f to 64ab6be Compare June 27, 2024 12:25

cameel reviewed Jun 27, 2024

View reviewed changes

cameel previously approved these changes Jun 28, 2024

View reviewed changes

clonker dismissed cameel’s stale review via 1492eb5 July 1, 2024 10:00

ekpyron reviewed Jul 1, 2024

View reviewed changes

clonker force-pushed the asm_parser_use_scanner branch from 96c162f to 7086538 Compare July 1, 2024 12:27

ekpyron reviewed Jul 1, 2024

View reviewed changes

ekpyron approved these changes Jul 1, 2024

View reviewed changes

Scanner: Add SpecialComment mode

c012b72

clonker added 2 commits July 1, 2024 18:29

AsmParser: Use scanner instead of regex for source comment parsing

ef9aa90

CI: Re-enable *_bytecode_too_large_* and *_combined_too_large* tests

8b0bb61

clonker force-pushed the asm_parser_use_scanner branch from 7086538 to 8b0bb61 Compare July 1, 2024 16:30

clonker merged commit 8d788fb into ethereum:develop Jul 1, 2024
72 checks passed

clonker deleted the asm_parser_use_scanner branch July 1, 2024 17:29

clonker mentioned this pull request Jul 2, 2024

Long statements can cause segmentation faults in IR-based codegen #15237

Closed

cameel mentioned this pull request Aug 30, 2024

Add support for parsing debug data attributes #14857

Open

2 tasks

Marenz mentioned this pull request Sep 11, 2024

Extremely long SPDX comment results in a segfault #12208

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AsmParser: parse source comment using scanner instead of regex #15209

AsmParser: parse source comment using scanner instead of regex #15209

clonker commented Jun 20, 2024 •

edited

Loading

matheusaaguiar left a comment

matheusaaguiar commented Jun 20, 2024 •

edited

Loading

cameel left a comment

aarlt commented Jun 25, 2024

cameel commented Jun 26, 2024

clonker commented Jun 27, 2024

aarlt commented Jun 27, 2024

aarlt commented Jun 27, 2024 •

edited

Loading

cameel left a comment

cameel Jun 27, 2024

clonker Jun 28, 2024 •

edited

Loading

cameel Jun 28, 2024

cameel commented Jun 27, 2024 •

edited

Loading

cameel left a comment

cameel Jun 28, 2024

clonker Jul 1, 2024

ekpyron Jul 1, 2024

clonker Jul 1, 2024

ekpyron Jul 1, 2024 •

edited

Loading

clonker Jul 1, 2024 •

edited

Loading

ekpyron Jul 1, 2024

ekpyron Jul 1, 2024

clonker Jul 1, 2024 •

edited

Loading

ekpyron Jul 1, 2024

ekpyron Jul 1, 2024

ekpyron Jul 1, 2024

clonker Jul 1, 2024 •

edited

Loading

clonker Jul 1, 2024

ekpyron Jul 1, 2024

ekpyron left a comment

aarlt commented Jul 1, 2024 •

edited

Loading

AsmParser: parse source comment using scanner instead of regex #15209

AsmParser: parse source comment using scanner instead of regex #15209

Conversation

clonker commented Jun 20, 2024 • edited Loading

matheusaaguiar left a comment

Choose a reason for hiding this comment

matheusaaguiar commented Jun 20, 2024 • edited Loading

cameel left a comment

Choose a reason for hiding this comment

aarlt commented Jun 25, 2024

cameel commented Jun 26, 2024

clonker commented Jun 27, 2024

aarlt commented Jun 27, 2024

aarlt commented Jun 27, 2024 • edited Loading

cameel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clonker Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cameel commented Jun 27, 2024 • edited Loading

cameel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekpyron Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

clonker Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clonker Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clonker Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekpyron left a comment

Choose a reason for hiding this comment

aarlt commented Jul 1, 2024 • edited Loading

clonker commented Jun 20, 2024 •

edited

Loading

matheusaaguiar commented Jun 20, 2024 •

edited

Loading

aarlt commented Jun 27, 2024 •

edited

Loading

clonker Jun 28, 2024 •

edited

Loading

cameel commented Jun 27, 2024 •

edited

Loading

ekpyron Jul 1, 2024 •

edited

Loading

clonker Jul 1, 2024 •

edited

Loading

clonker Jul 1, 2024 •

edited

Loading

clonker Jul 1, 2024 •

edited

Loading

aarlt commented Jul 1, 2024 •

edited

Loading