[llvm-ml] Remove unsafe getCurrentSegmentOnly() call

[ericastor]

This call was made unsafe recently, but was not fixed in db48f1a (the commit that fixed the parallel code in AsmParser.cpp).

Fixes #123189

[llvmbot]

@llvm/pr-subscribers-mc

Author: Eric Astor (ericastor)

Changes

This call was made unsafe recently, but was not fixed in db48f1a (the commit that fixed the parallel code in AsmParser.cpp).

---

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

1 Files Affected:

• (modified) llvm/lib/MC/MCParser/MasmParser.cpp (+1-1)

diff --git a/llvm/lib/MC/MCParser/MasmParser.cpp b/llvm/lib/MC/MCParser/MasmParser.cpp
index 78261c1f9fedb2..6c49a17180f27b 100644
--- a/llvm/lib/MC/MCParser/MasmParser.cpp
+++ b/llvm/lib/MC/MCParser/MasmParser.cpp
@@ -1454,7 +1454,7 @@ bool MasmParser::Run(bool NoInitialTextSection, bool NoFinalize) {
 }
 
 bool MasmParser::checkForValidSection() {
-  if (!ParsingMSInlineAsm && !getStreamer().getCurrentSectionOnly()) {
+  if (!ParsingMSInlineAsm && !getStreamer().getCurrentFragment()) {
     Out.initSections(false, getTargetParser().getSTI());
     return Error(getTok().getLoc(),
                  "expected section directive before assembly directive");

[MaskRay]

Could you add a test?

[ericastor]

I can try to minimize the issue & convert it into a test, but I'm not quite clear on what circumstances actually provoke it. For example, neither db48f1a nor 626eef5 seemed to include a test to verify that their changes were separately safe?

[ericastor]

Added tests as requested.

[MaskRay]

The two commits intended to simplify MC internals.
For the gas compatible AsmParser.cpp, we have comprehensive tests and passing the testsuite gives contributors very high confidence in the patches.
I apologize if the simplification actually caused regression to MasmParser.cpp. That was not my intention.

That said, I should be honest. I am deeply concerned with the state of MasmParser.cpp.
When MasmParser.cpp was forked from AsmParser.cpp, it copied a lot of code from AsmParser.
A lot of pieces are unreachable or untested for llvm-ml.

I don't think it is reasonable to require contributors that refactor MC to understand
llvm-ml internals and make perfect judgement on whether a refactoring will regress llvm-ml or not.
You have to write tests to guard against regressions.

[MaskRay]

The convention is to just state expected something and omit before directive .... The before directive information is obvious from the source line that is printed

[ericastor]

Revised accordingly.

[ericastor]

The two commits intended to simplify MC internals. For the gas compatible AsmParser.cpp, we have comprehensive tests and passing the testsuite gives contributors very high confidence in the patches. I apologize if the simplification actually caused regression to MasmParser.cpp. That was not my intention.

That said, I should be honest. I am deeply concerned with the state of MasmParser.cpp. When MasmParser.cpp was forked from AsmParser.cpp, it copied a lot of code from AsmParser. A lot of pieces are unreachable or untested for llvm-ml.

I don't think it is reasonable to require contributors that refactor MC to understand llvm-ml internals and make perfect judgement on whether a refactoring will regress llvm-ml or not. You have to write tests to guard against regressions.

I agree. To be honest, I was surprised the test suite didn't already cover this case, so this was definitely a mistake on my part. I'm just interested which tests (EDIT: for the base AsmParser) detected the issue with this refactor of MC, but I'm pleased to hear that they were broken and detected the issue before submission.

In terms of reducing MasmParser.cpp... I agree that it could use a lot of cleanup. I would very much like to work on improving the test case coverage. Is there an easy way to get coverage reporting out of the LLVM test suite? I'm apparently not familiar with it, and since it's very much designed as more of an integration-test framework, I don't know any obvious tricks.

[MaskRay]

omit braces for the single line simple statement

[MaskRay]

This is ok, but in the majority of tests we omit spaces beside +: LINE+1