[PATCH] D79570: [MC] Fix PR45805: infinite recursion in assembler

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

Thanks for the patch. This approach may be fine.



================
Comment at: llvm/include/llvm/MC/MCAsmLayout.h:57
+  bool isFragmentValidating(const MCFragment *F) const {
+    return LayingOutFragmentsSet.find(F) != LayingOutFragmentsSet.end();
+  }
----------------
count

Why is called Validating instead of layouting?


================
Comment at: llvm/lib/MC/MCAssembler.cpp:401
+  bool InsertionSuccess{};
+  std::tie(std::ignore, InsertionSuccess) = LayingOutFragmentsSet.insert(F);
+  assert(InsertionSuccess && "Already being laid out!");
----------------
```
bool Inserted = LayingOutFragmentsSet.insert(F).second;
assert(Inserted && "Already being laid out!");
(void)Inserted;
```


================
Comment at: llvm/lib/MC/MCExpr.cpp:581
 
+  if (Layout &&
+      (Layout->isFragmentValidating(FA) || Layout->isFragmentValidating(FB)))
----------------
Add a comment along the lines of: the enclosing fragment is being layouted. Avoid self loop.


================
Comment at: llvm/test/MC/AsmParser/pr45805.s:1
+// RUN: not llvm-mc --filetype=obj %s -o /dev/null 2>&1 | FileCheck %s
+
----------------
`# ` is simpler


================
Comment at: llvm/test/MC/AsmParser/pr45805.s:1
+// RUN: not llvm-mc --filetype=obj %s -o /dev/null 2>&1 | FileCheck %s
+
----------------
MaskRay wrote:
> `# ` is simpler
binutils use prXXXX.s for regression tests. I find the names not very descriptive. How about `layout-*.s`? I currently haven't thought of a good name for the `-*` part.


================
Comment at: llvm/test/MC/AsmParser/pr45805.s:7
+// CHECK: pr45805.s:7:7: error: expected assembly-time absolute expression
+.fill (data_start - fct_end), 1, 42
+
----------------
The test covers `Layout->isFragmentValidating(FA) `. You also need 

`.fill (fct_end - data_start), 1, 42`

to test `Layout->isFragmentValidating(FB)`


================
Comment at: llvm/test/MC/AsmParser/pr45805.s:10
+data_start:
+.long 0x42424242
----------------
Delete the unneeded .long


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79570/new/

https://reviews.llvm.org/D79570