[PATCH] D95851: [MC][ELF] Support for zero flag section groups

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

Mostly LG.



================
Comment at: llvm/include/llvm/MC/MCContext.h:490
                                 unsigned Flags, unsigned EntrySize,
-                                const Twine &Group) {
-      return getELFSection(Section, Type, Flags, EntrySize, Group,
+                                const Twine &Group, bool IsComdat = true) {
+      return getELFSection(Section, Type, Flags, EntrySize, Group, IsComdat,
----------------
phosek wrote:
> MaskRay wrote:
> > phosek wrote:
> > > MaskRay wrote:
> > > > `Group == ""` means a non-SHF_GROUP section. `IsComdat = true` can be misleading for such use cases.
> > > > 
> > > > We should pick a variable which defaults to false here.
> > > > 
> > > > NoDeduplicates is a PE-COFF specific name, and we should stick with ELF naming here. So a concise name indicating zero section group flags should be used.
> > > I used `IsComdat = true` because I was worried that updating every existing call site is going to be too much effort, but turned it wasn't so bad so I removed the default argument.
> > > 
> > > I think that having `/*IsComdat=*/true` or `/*IsComdat=*/false` explicitly in the code is actually nice because it's clear which type of group we're requesting.
> > > 
> > > Let me know what you think.
> > If you flip the default to `/*IsComdat=*/false`, it looks good to me.
> > 
> > That means a generic section has `Group=="" && !IsComdat`.
> > A comdat section needs `Group!="" && IsComdat`.
> This is done in the latest revision.
Thanks. The large number of getELFSection overloads is somewhat difficult to manage...I deleted some overloads. Someone may want to further clean up them...


================
Comment at: llvm/test/CodeGen/X86/group.ll:1
+; RUN: llc < %s -mtriple=x86_64-unknown-linux | FileCheck %s
+
----------------
Add a file-level comment explaining the purpose of the test.


================
Comment at: llvm/test/CodeGen/X86/group.ll:3
+
+; CHECK: .section .text.f1,"axG", at progbits,f1
+; CHECK: .section .text.f2,"axG", at progbits,f1
----------------
Add `{{$}}` otherwise they don't catch accidental `,comdat`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95851