[PATCH] D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:723
+  if (getContext().getAsmInfo()->useIntegratedAssembler()) {
+    // Mergeable symbols should always be placed into mergeable sections due to
+    // the UniqueID code above.
----------------
The two assert may not be that useful. They can probably be deleted.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:743
+        (Section->getEntrySize() != getEntrySizeForKind(Kind)))
+      report_fatal_error(
+          "Symbol '" + GO->getName() + "' from module '" +
----------------
nickdesaulniers wrote:
> If we have a way of producing a non-fatal warning, that may be preferred to crashing the compiler, which encourages users to send us a bug report. In this case, we don't care about their buggy code with impossible to solve constraints, and the text you provide here should be enough information for them to solve their code.
> 
> Maybe a warning, but proceed with codegen? The warning can encourage them that referencing those variables is undefined behavior.
This code path is possible, which means we should use MCContext::reportError rather than report_fatal_error. In a bugfree world, report_fatal_error should not ever be called.


================
Comment at: llvm/test/CodeGen/X86/explict-section-mergeable.ll:266
+ at explicit_asm_1 = unnamed_addr constant [2 x i16] [i16 1, i16 0], section ".asm_mergeable1"
+ at explicit_asm_2 = unnamed_addr constant [2 x i16] [i16 1, i16 0], section ".asm_nonmergeable1"
+
----------------
Changing to 1 x i32 can enhance the test.


================
Comment at: llvm/test/CodeGen/X86/explict-section-mergeable.ll:273
+;; Assign compatible and incompatible globals to the same sections.
+ at explicit_asm_5 = unnamed_addr constant [2 x i16] [i16 1, i16 0], section ".asm_mergeable2"
+ at explicit_asm_6 = unnamed_addr constant [2 x i16] [i16 1, i16 0], section ".asm_nonmergeable2"
----------------
explicit_asm_5 and explicit_asm_6 can be deleted. As a replacement, you can add another constant following explicit_asm_2 which is compatible with explicit_asm_[12]


================
Comment at: llvm/test/CodeGen/X86/explict-section-mergeable.ll:280
+;; --implicit-check-not to reduce the number of checks in this file.
+; CHECK: .section ".note.GNU-stack","", at progbits
+
----------------
.note.GNU-stack is unrelated to the test.


================
Comment at: llvm/test/CodeGen/X86/explict-section-mergeable.ll:284
+
+; RUN: not --crash llc < %s -mtriple=x86_64-linux-gnu --no-integrated-as 2>&1 \
+; RUN:     | FileCheck %s --check-prefix=NO-I-ASM
----------------
Delete `not --crash`. llc --no-integrated-as should not crash.


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

https://reviews.llvm.org/D72194