[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/include/llvm/MC/MCContext.h:20
 #include "llvm/ADT/Twine.h"
+#include "llvm/BinaryFormat/ELF.h"
 #include "llvm/BinaryFormat/Dwarf.h"
----------------
ELF.h should be after Dwarf.h


================
Comment at: llvm/test/CodeGen/X86/explict-section-mergeable.ll:1
+; RUN: llc < %s -mtriple=x86_64-linux-gnu -unique-section-names=0 -data-sections 2>&1 \
+; RUN:     | FileCheck %s
----------------
Nit: `-linux-gnu` can be deleted.


================
Comment at: llvm/test/CodeGen/X86/explict-section-mergeable.ll:1
+; RUN: llc < %s -mtriple=x86_64-linux-gnu -unique-section-names=0 -data-sections 2>&1 \
+; RUN:     | FileCheck %s
----------------
MaskRay wrote:
> Nit: `-linux-gnu` can be deleted.
I just realized the file name made a typo. It should be `explicit`


================
Comment at: llvm/test/CodeGen/X86/explict-section-mergeable.ll:16
+
+;; Create a uniquified symbol (as -unique-section-names=0) to test the uniqueID 
+;; interaction with mergeable symbols.
----------------
Delete trailing whitespace


================
Comment at: llvm/test/CodeGen/X86/explict-section-mergeable.ll:46
+;; Assign a compatible mergeable global to the previous section.
+ at explicit_basic_2 = unnamed_addr constant [2 x i16] [i16 1, i16 1], section ".explicit_basic"
+
----------------
Changing 2 x i16 to 1 x i32 can enhance the test.


================
Comment at: llvm/test/CodeGen/X86/explict-section-mergeable.ll:62
+;; Assign a symbol with an incompatible entsize (string vs non-string) to a section with the same name.
+ at explicit_basic_5 = unnamed_addr constant [2 x i32] [i32 1, i32 0], section ".explicit_basic"
+;; Assign a compatible mergeable global to the previous section.
----------------
explicit_basic_5 does not test string vs non-string.


================
Comment at: llvm/test/CodeGen/X86/explict-section-mergeable.ll:64
+;; Assign a compatible mergeable global to the previous section.
+ at explicit_basic_6 = unnamed_addr constant [2 x i32] [i32 1, i32 0], section ".explicit_basic"
+
----------------
Change to 1 x i64 to enhance the test.


================
Comment at: llvm/test/CodeGen/X86/explict-section-mergeable.ll:70
+;; Assign a symbol with an incompatible entsize (non-mergeable) to a mergeable section created explicitly.
+ at explicit_basic_7 = constant [2 x i16] [i16 1, i16 1], section ".explicit_basic"
+
----------------
This can be deleted.

If you want to test a non-unnamed_addr constant, this should be 2 x i32 and the comment should be fixed.


================
Comment at: llvm/test/CodeGen/X86/explict-section-mergeable.ll:108
+;; Non-allocatable "default" sections can have allocatable mergeable symbols with compatible entry sizes assigned to them.
+ at explicit_default_3 = unnamed_addr constant [2 x i8] [i8 1, i8 0], section ".debug_str"
+
----------------
This test does not seem correct.

.debug_str is a pre-allocated non-SHF_ALLOC section. A constant (SHF_ALLOC) cannot reuse the section.


================
Comment at: llvm/test/CodeGen/X86/explict-section-mergeable.ll:114
+;; Non-allocatable "default" sections cannot have allocatable mergeable symbols with incompatible (non-mergeable) entry sizes assigned to them.
+ at explicit_default_4 = constant [2 x i16] [i16 1, i16 1], section ".debug_str"
+
----------------
This non-unnamed_addr test does not add coverage.


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

https://reviews.llvm.org/D72194