Implement llvm.commandline named metadata

[scott.linder]

Summary

Implement a new special named metadata, llvm.commandline, to support frontends embedding their command-line options in IR/ASM/code-objects. Specifically this supports adding the -frecord-gcc-switches option to Clang.

This differs from the GCC implementation in a few ways, and I am looking for feedback on whether these differences are reasonable/correct.

In GCC there is only one command-line possible per compilation-unit, in LLVM it mirrors llvm.ident and multiple are allowed.

For the ELF section (".GCC.command.line"), in the case of GCC the options are null-terminated (e.g. '-foo\0-bar\0-baz\0'); in LLVM, in order to differentiate multiple merged command-lines, the null byte instead separates entire command-lines (e.g. '\0compiler1 -foo -bar -baz\0compiler2 -qux\0'). This is the biggest departure in terms of implementation, but in the face of section merging I don't understand how the GCC approach works in any meaningful way. The advantage I see to the GCC approach is unambiguously separating individual arguments without relying on shell-style parsing.

[JohnReagan]

I know you are just trying to match gcc behavior, but do you know why they use SHT_PROGBITS instead of something like SHT_NOTE for the information? On OpenVMS, we use .note entries to record compilation date/time, compiler version, command line, etc. information that is later used by our linker to print out detail link maps. Or is the intention that the information get propagated all the way to the final executable or shared library?

[scott.linder]

In D54487#1297461, @JohnReagan wrote:

I know you are just trying to match gcc behavior, but do you know why they use SHT_PROGBITS instead of something like SHT_NOTE for the information? On OpenVMS, we use .note entries to record compilation date/time, compiler version, command line, etc. information that is later used by our linker to print out detail link maps. Or is the intention that the information get propagated all the way to the final executable or shared library?

I don't have a good answer as to why GCC decided to use SHT_PROGBITS, and you're correct that I simply followed their lead. I think SHT_NOTE seems more appropriate, although at AMD we do want to propagate the section to the final executable/shared object; are the two incompatible? It seems fine if tools like strip remove the section.

[probinson]

I don't really see a need for a new directive, it's convenient but does nothing you couldn't do with existing directives.
It does have some downside, in that it's supported only by the LLVM integrated assembler.

[manojgupta]

Doesn't clang already has -grecord-gcc-switches option (https://clang.llvm.org/docs/ClangCommandLineReference.html#debug-information-flags) ?

[scott.linder]

In D54487#1297520, @probinson wrote:

I don't really see a need for a new directive, it's convenient but does nothing you couldn't do with existing directives.
It does have some downside, in that it's supported only by the LLVM integrated assembler.

That's very reasonable; I will update the patch to remove the new directive.

In D54487#1297947, @manojgupta wrote:

Doesn't clang already has -grecord-gcc-switches option (https://clang.llvm.org/docs/ClangCommandLineReference.html#debug-information-flags) ?

Yes, but the -g and -f variants have different purposes. This patch adds the -f version to embed the flags in e.g. an ELF section, rather than the -g version which embeds it in e.g. DWARF.

[JohnReagan]

In D54487#1297507, @scott.linder wrote:

In D54487#1297461, @JohnReagan wrote:

I know you are just trying to match gcc behavior, but do you know why they use SHT_PROGBITS instead of something like SHT_NOTE for the information? On OpenVMS, we use .note entries to record compilation date/time, compiler version, command line, etc. information that is later used by our linker to print out detail link maps. Or is the intention that the information get propagated all the way to the final executable or shared library?

I don't have a good answer as to why GCC decided to use SHT_PROGBITS, and you're correct that I simply followed their lead. I think SHT_NOTE seems more appropriate, although at AMD we do want to propagate the section to the final executable/shared object; are the two incompatible? It seems fine if tools like strip remove the section.

Got it. Yes, SHT_NOTEs probably don't survive into the final image so SHT_PROGBITS is what was chosen. I wonder if the linker special cases on the section name to keep it from being part of a load section? Seems like a waste to read that stuff into memory at image activation time.

[JohnReagan]

In D54487#1297947, @manojgupta wrote:

Doesn't clang already has -grecord-gcc-switches option (https://clang.llvm.org/docs/ClangCommandLineReference.html#debug-information-flags) ?

Yes, but that is somewhat different. The "-g" option seems to just append the command line to the producer string, yes? It is harder to parse out later and it can be stripped out. gcc has both -g and -f forms.

[manojgupta]

In D54487#1298688, @JohnReagan wrote:

Yes, but that is somewhat different. The "-g" option seems to just append the command line to the producer string, yes? It is harder to parse out later and it can be stripped out. gcc has both -g and -f forms.

Thanks, I was confused given the similarity of option strings.

[scott.linder]

Remove new ".commandline" ASM directive in favor of standard ones (e.g. .section, .ascii)

[scott.linder]

Rebase

[scott.linder]

In D54487#1298669, @JohnReagan wrote:

Got it. Yes, SHT_NOTEs probably don't survive into the final image so SHT_PROGBITS is what was chosen. I wonder if the linker special cases on the section name to keep it from being part of a load section? Seems like a waste to read that stuff into memory at image activation time.

As far as I understand it the SHT_PROGBITS won't cause the section to be loaded if it is not also SHF_ALLOC, but I have not actually confirmed this.

[scott.linder]

Ping

[scott.linder]

I'm not certain who else might want to review this, but I'm adding some who have reviewed the touched files recently.

[JohnReagan]

I'll ask the larger question of why did you pick ".LLVM.command.line" as the section name? While distasteful, there might be existing scripts that do things like:

readelf -p .GCC.command.line a.out

to dump out the arguments like

String dump of section '.GCC.command.line':

[     0]  a.c
[     4]  -mtune=generic
[    13]  -march=x86-64
[    21]  -O2
[    25]  -frecord-gcc-switches

Yes, the format of contents is not well-defined (as mentioned in the gcc man pages), but for somebody switching from a gcc-based toolchain to an LLVM-based toolchain, would they expect the sections to have the same names?

Your description says this would allow clang to add -frecord-gcc-switches (and not -frecord-llvm-switches) so would someone expect that clang would follow the module and create a '.GCC.command.line' section name?

Or perhaps also create a .GCC.command.line section that would contain 'see .LLVM.command.line for additional command line descriptions' so at least existing scripts might not get tripped up?

Or am I overthinking this from working in a high "upward-compatibility, don't break anything" environment for 30+ years?

[scott.linder]

In D54487#1325584, @JohnReagan wrote:

I'll ask the larger question of why did you pick ".LLVM.command.line" as the section name? While distasteful, there might be existing scripts that do things like:

readelf -p .GCC.command.line a.out

to dump out the arguments like

String dump of section '.GCC.command.line':

[     0]  a.c
[     4]  -mtune=generic
[    13]  -march=x86-64
[    21]  -O2
[    25]  -frecord-gcc-switches

Yes, the format of contents is not well-defined (as mentioned in the gcc man pages), but for somebody switching from a gcc-based toolchain to an LLVM-based toolchain, would they expect the sections to have the same names?

Your description says this would allow clang to add -frecord-gcc-switches (and not -frecord-llvm-switches) so would someone expect that clang would follow the module and create a '.GCC.command.line' section name?

Or perhaps also create a .GCC.command.line section that would contain 'see .LLVM.command.line for additional command line descriptions' so at least existing scripts might not get tripped up?

Or am I overthinking this from working in a high "upward-compatibility, don't break anything" environment for 30+ years?

Looking at it again I think I agree that changing the name is not as useful as I had intended, and probably does more harm than it is worth. I originally wanted to avoid breaking the "ABI" of the section, for tools which go beyond inspecting it visually and rely on a certain binary layout (as that layout is changing), but if that format is not well-defined anyway I think preserving the name is reasonable. I will update both patches, and will be explicit in the Clang docs about the change in format.

[scott.linder]

Use the same ELF section name as GCC

[probinson]

Does the verifier bail out after the first failure? If not, you can combine the failure tests all into one test.

lib/CodeGen/AsmPrinter/AsmPrinter.cpp

2021

You can reduce some indentation by consistently using the early-return idiom:

const NamedMDNode *NMD = ...;
if (!NMD || !NMD->getNumOperands())
  return;
// rest of function

test/Linker/commandline.ll

9

You can make sure that each of the strings has one of the expected MD numbers:

CHECK-DAG: !{{[0-2]}} = !{!"compiler -v1"}

etc.

test/Verifier/commandline-meta1.ll

8

I think it would be ok (and more obvious) if you omit the valid entry.

test/Verifier/commandline-meta2.ll

10

Simpler and more obvious if you omit the valid entries.

[JohnReagan]

lib/CodeGen/TargetLoweringObjectFileImpl.cpp

799

So now that we've switched to a section name with GCC, do you want a comment so any future contributor won't be confused? If you only look at the code without these review comments (or the corresponding clang edits), you might wonder about the overall design goals. Something like

// Use ".GCC.command.line" since this feature is to support 
// clang's -frecord-gcc-switches which in turn attempts to 
// mimic GCC's switch of the same name.

[scott.linder]

Address feedback. The verifier appears to bail on the first failure, at least with llc/llvm-as.

[probinson]

LGTM although I think John should have a chance to get in a last word.

[JohnReagan]

LGTM2

[MaskRay]

cfe/trunk/test/Driver/debug-options.c

161 ↗

(On Diff #178233)

As the continuation line doesn't use RUN:, this command will always fail. We don't detect this error because -### subsumes many errors and return with 0. Caught by D156363

include/llvm/CodeGen/AsmPrinter.h

private:
 
   void EmitLinkage(const GlobalValue *GV, MCSymbol *GVSym) const;
 
   void EmitJumpTableEntry(const MachineJumpTableInfo *MJTI,
                           const MachineBasicBlock *MBB, unsigned uid) const;
   void EmitLLVMUsedList(const ConstantArray *InitList);
   /// Emit llvm.ident metadata in an '.ident' directive.
   void EmitModuleIdents(Module &M);
+  /// Emit bytes for llvm.commandline metadata.
+  void EmitModuleCommandLines(Module &M);
   void EmitXXStructorList(const DataLayout &DL, const Constant *List,
                           bool isCtor);
 
   GCMetadataPrinter *GetOrCreateGCPrinter(GCStrategy &S);
   /// Emit GlobalAlias or GlobalIFunc.
   void emitGlobalIndirectSymbol(Module &M, const GlobalIndirectSymbol &GIS);
   void setupCodePaddingContext(const MachineBasicBlock &MBB,
                                MCCodePaddingContext &Context) const;
 };
 
 } // end namespace llvm
 
 #endif // LLVM_CODEGEN_ASMPRINTER_H

include/llvm/CodeGen/TargetLoweringObjectFileImpl.h

public:
   MCSection *getStaticCtorSection(unsigned Priority,
                                   const MCSymbol *KeySym) const override;
   MCSection *getStaticDtorSection(unsigned Priority,
                                   const MCSymbol *KeySym) const override;
 
   const MCExpr *lowerRelativeReference(const GlobalValue *LHS,
                                        const GlobalValue *RHS,
                                        const TargetMachine &TM) const override;
+
+  MCSection *getSectionForCommandLines() const override;
 };
 
 class TargetLoweringObjectFileMachO : public TargetLoweringObjectFile {
 public:
   TargetLoweringObjectFileMachO();
   ~TargetLoweringObjectFileMachO() override = default;
 
   void Initialize(MCContext &Ctx, const TargetMachine &TM) override;

include/llvm/Target/TargetLoweringObjectFile.h

public:
   }
 
   virtual void emitLinkerFlagsForGlobal(raw_ostream &OS,
                                         const GlobalValue *GV) const {}
 
   virtual void emitLinkerFlagsForUsed(raw_ostream &OS,
                                       const GlobalValue *GV) const {}
 
+  /// If supported, return the section to use for the llvm.commandline
+  /// metadata. Otherwise, return nullptr.
+  virtual MCSection *getSectionForCommandLines() const {
+    return nullptr;
+  }
+
 protected:
   virtual MCSection *SelectSectionForGlobal(const GlobalObject *GO,
                                             SectionKind Kind,
                                             const TargetMachine &TM) const = 0;
 };
 
 } // end namespace llvm
 
 #endif // LLVM_CODEGEN_TARGETLOWERINGOBJECTFILE_H

lib/CodeGen/AsmPrinter/AsmPrinter.cpp

bool AsmPrinter::doFinalization(Module &M) {
   assert(MI && "AsmPrinter didn't require GCModuleInfo?");
   for (GCModuleInfo::iterator I = MI->end(), E = MI->begin(); I != E; )
     if (GCMetadataPrinter *MP = GetOrCreateGCPrinter(**--I))
       MP->finishAssembly(M, *MI, *this);
 
   // Emit llvm.ident metadata in an '.ident' directive.
   EmitModuleIdents(M);
 
+  // Emit bytes for llvm.commandline metadata.
+  EmitModuleCommandLines(M);
+
   // Emit __morestack address if needed for indirect calls.
   if (MMI->usesMorestackAddr()) {
     unsigned Align = 1;
     MCSection *ReadOnlySection = getObjFileLowering().getSectionForConstant(
         getDataLayout(), SectionKind::getReadOnly(),
         /*C=*/nullptr, Align);
     OutStreamer->SwitchSection(ReadOnlySection);
 
for (unsigned i = 0, e = NMD->getNumOperands(); i != e; ++i) {
       assert(N->getNumOperands() == 1 &&
              "llvm.ident metadata entry can have only one operand");
       const MDString *S = cast<MDString>(N->getOperand(0));
       OutStreamer->EmitIdent(S->getString());
     }
   }
 }
 
+void AsmPrinter::EmitModuleCommandLines(Module &M) {
+  MCSection *CommandLine = getObjFileLowering().getSectionForCommandLines();
+  if (!CommandLine)
+    return;
+
+  const NamedMDNode *NMD = M.getNamedMetadata("llvm.commandline");
+  if (!NMD || !NMD->getNumOperands())
+    return;
+
+  OutStreamer->PushSection();
+  OutStreamer->SwitchSection(CommandLine);
+  OutStreamer->EmitZeros(1);
+  for (unsigned i = 0, e = NMD->getNumOperands(); i != e; ++i) {
+    const MDNode *N = NMD->getOperand(i);
+    assert(N->getNumOperands() == 1 &&
+           "llvm.commandline metadata entry can have only one operand");
+    const MDString *S = cast<MDString>(N->getOperand(0));
+    OutStreamer->EmitBytes(S->getString());
+    OutStreamer->EmitZeros(1);
+  }
+  OutStreamer->PopSection();
+}
+
 //===--------------------------------------------------------------------===//
 // Emission and print routines
 //
 
 /// Emit a byte directive and value.
 ///
 void AsmPrinter::emitInt8(int Value) const {
   OutStreamer->EmitIntValue(Value, 1);

lib/CodeGen/TargetLoweringObjectFileImpl.cpp

if (LHS->getType()->getPointerAddressSpace() != 0 ||
     return nullptr;
 
   return MCBinaryExpr::createSub(
       MCSymbolRefExpr::create(TM.getSymbol(LHS), PLTRelativeVariantKind,
                               getContext()),
       MCSymbolRefExpr::create(TM.getSymbol(RHS), getContext()), getContext());
 }
 
+MCSection *TargetLoweringObjectFileELF::getSectionForCommandLines() const {
+  // Use ".GCC.command.line" since this feature is to support clang's
+  // -frecord-gcc-switches which in turn attempts to mimic GCC's switch of the
+  // same name.
+  return getContext().getELFSection(".GCC.command.line", ELF::SHT_PROGBITS,
+                                    ELF::SHF_MERGE | ELF::SHF_STRINGS, 1, "");
+}
+
 void
 TargetLoweringObjectFileELF::InitializeELF(bool UseInitArray_) {
   UseInitArray = UseInitArray_;
   MCContext &Ctx = getContext();
   if (!UseInitArray) {
     StaticCtorSection = Ctx.getELFSection(".ctors", ELF::SHT_PROGBITS,
                                           ELF::SHF_ALLOC | ELF::SHF_WRITE);
 

lib/IR/Verifier.cpp

bool verify() {
     for (const NamedMDNode &NMD : M.named_metadata())
       visitNamedMDNode(NMD);
 
     for (const StringMapEntry<Comdat> &SMEC : M.getComdatSymbolTable())
       visitComdat(SMEC.getValue());
 
     visitModuleFlags(M);
     visitModuleIdents(M);
+    visitModuleCommandLines(M);
 
     verifyCompileUnits();
 
     verifyDeoptimizeCallingConvs();
     DISubprogramAttachments.clear();
     return !Broken;
   }
 
 private:
   // Verification methods...
   void visitGlobalValue(const GlobalValue &GV);
   void visitGlobalVariable(const GlobalVariable &GV);
   void visitGlobalAlias(const GlobalAlias &GA);
   void visitAliaseeSubExpr(const GlobalAlias &A, const Constant &C);
   void visitAliaseeSubExpr(SmallPtrSetImpl<const GlobalAlias *> &Visited,
                            const GlobalAlias &A, const Constant &C);
   void visitNamedMDNode(const NamedMDNode &NMD);
   void visitMDNode(const MDNode &MD);
   void visitMetadataAsValue(const MetadataAsValue &MD, Function *F);
   void visitValueAsMetadata(const ValueAsMetadata &MD, Function *F);
   void visitComdat(const Comdat &C);
   void visitModuleIdents(const Module &M);
+  void visitModuleCommandLines(const Module &M);
   void visitModuleFlags(const Module &M);
   void visitModuleFlag(const MDNode *Op,
                        DenseMap<const MDString *, const MDNode *> &SeenIDs,
                        SmallVectorImpl<const MDNode *> &Requirements);
   void visitModuleFlagCGProfileEntry(const MDOperand &MDO);
   void visitFunction(const Function &F);
   void visitBasicBlock(BasicBlock &BB);
   void visitRangeMetadata(Instruction &I, MDNode *Range, Type *Ty);
Assert(N->getNumOperands() == 1,
            "incorrect number of operands in llvm.ident metadata", N);
     Assert(dyn_cast_or_null<MDString>(N->getOperand(0)),
            ("invalid value for llvm.ident metadata entry operand"
             "(the operand should be a string)"),
            N->getOperand(0));
   }
 }
 
+void Verifier::visitModuleCommandLines(const Module &M) {
+  const NamedMDNode *CommandLines = M.getNamedMetadata("llvm.commandline");
+  if (!CommandLines)
+    return;
+
+  // llvm.commandline takes a list of metadata entry. Each entry has only one
+  // string. Scan each llvm.commandline entry and make sure that this
+  // requirement is met.
+  for (const MDNode *N : CommandLines->operands()) {
+    Assert(N->getNumOperands() == 1,
+           "incorrect number of operands in llvm.commandline metadata", N);
+    Assert(dyn_cast_or_null<MDString>(N->getOperand(0)),
+           ("invalid value for llvm.commandline metadata entry operand"
+            "(the operand should be a string)"),
+           N->getOperand(0));
+  }
+}
+
 void Verifier::visitModuleFlags(const Module &M) {
   const NamedMDNode *Flags = M.getModuleFlagsMetadata();
   if (!Flags) return;
 
   // Scan each flag, and track the flags and requirements.
   DenseMap<const MDString*, const MDNode*> SeenIDs;
   SmallVector<const MDNode*, 16> Requirements;
   for (const MDNode *MDN : Flags->operands())

test/CodeGen/X86/commandline-metadata.ll

+; RUN: llc -mtriple=x86_64-linux < %s | FileCheck %s
+; Verify that llvm.commandline metadata is emitted to a section named
+; .GCC.command.line with each line separated with null bytes.
+
+; CHECK: .section .GCC.command.line,"MS",@progbits,1
+; CHECK-NEXT: .zero 1
+; CHECK-NEXT: .ascii "clang -command -line"
+; CHECK-NEXT: .zero 1
+; CHECK-NEXT: .ascii "something else"
+; CHECK-NEXT: .zero 1
+!llvm.commandline = !{!0, !1}
+!0 = !{!"clang -command -line"}
+!1 = !{!"something else"}

test/Linker/Inputs/commandline.a.ll

+!llvm.commandline = !{!0, !1}
+!0 = !{!"compiler -v1"}
+!1 = !{!"compiler -v2"}

test/Linker/Inputs/commandline.b.ll

+!llvm.commandline = !{!0}
+!0 = !{!"compiler -v3"}

test/Linker/commandline.ll

+; RUN: llvm-link %S/Inputs/commandline.a.ll %S/Inputs/commandline.b.ll -S | FileCheck %s
+
+; Verify that multiple input llvm.commandline metadata are linked together.
+
+; CHECK-DAG: !llvm.commandline = !{!0, !1, !2}
+; CHECK-DAG: !{{[0-2]}} = !{!"compiler -v1"}
+; CHECK-DAG: !{{[0-2]}} = !{!"compiler -v2"}
+; CHECK-DAG: !{{[0-2]}} = !{!"compiler -v3"}

test/Verifier/commandline-meta1.ll

+; RUN: not llvm-as < %s -o /dev/null 2>&1 | FileCheck %s
+; Verify that llvm.commandline is properly structured.
+; llvm.commandline takes a list of metadata entries.
+; Each metadata entry can have only one string.
+
+!llvm.commandline = !{!0}
+!0 = !{!"string1", !"string2"}
+; CHECK: assembly parsed, but does not verify as correct!
+; CHECK-NEXT: incorrect number of operands in llvm.commandline metadata
+; CHECK-NEXT: !0

test/Verifier/commandline-meta2.ll

+; RUN: not llvm-as < %s -o /dev/null 2>&1 | FileCheck %s
+; Verify that llvm.commandline is properly structured.
+; llvm.commandline takes a list of metadata entries.
+; Each metadata entry can contain one string only.
+
+!llvm.commandline = !{!0}
+!0 = !{i32 1}
+; CHECK: assembly parsed, but does not verify as correct!
+; CHECK-NEXT: invalid value for llvm.commandline metadata entry operand(the operand should be a string)
+; CHECK-NEXT: i32 1

test/Verifier/commandline-meta3.ll

+; RUN: not llvm-as < %s -o /dev/null 2>&1 | FileCheck %s
+; Verify that llvm.commandline is properly structured.
+; llvm.commandline takes a list of metadata entries.
+; Each metadata entry can contain one string only.
+
+!llvm.commandline = !{!0}
+!0 = !{!{!"nested metadata"}}
+; CHECK: assembly parsed, but does not verify as correct!
+; CHECK-NEXT: invalid value for llvm.commandline metadata entry operand(the operand should be a string)
+; CHECK-NEXT: !1

test/Verifier/commandline-meta4.ll

+; RUN: not llvm-as < %s -o /dev/null 2>&1 | FileCheck %s
+; Verify that llvm.commandline is properly structured.
+; llvm.commandline takes a list of metadata entries.
+; Each metadata entry can contain one string only.
+
+!llvm.commandline = !{!0}
+!0 = !{null}
+; CHECK: assembly parsed, but does not verify as correct!
+; CHECK-NEXT: invalid value for llvm.commandline metadata entry operand(the operand should be a string)