[Metadata] Introduce MD_pcsections

[melver]

Summary

Introduces MD_pcsections metadata kind. See added documentation for
more details.

Subsequent patches enable propagating PC sections metadata through code
generation to the AsmPrinter.

RFC: https://discourse.llvm.org/t/rfc-pc-keyed-metadata-at-runtime/64191

[melver]

Associated RFC of this series: https://discourse.llvm.org/t/rfc-pc-keyed-metadata-at-runtime/64191

[dvyukov]

llvm/docs/PCSectionsMetadata.rst

37–38

There seems to be something with wording:
"backend to emit the PC ... to be emitted to all named sections"
"backend to emit the PC ... to all named sections"?

[melver]

Fix typos.

[MaskRay]

This introduces new LangRef constructs. I suggest that you enlarge the reviewer list.

In D130875#3690485, @melver wrote:

Associated RFC of this series: https://discourse.llvm.org/t/rfc-pc-keyed-metadata-at-runtime/64191

This link can be amended into the summary (commit message).

[MaskRay]

llvm/test/CodeGen/AArch64/pcsections.ll

1 ↗

(On Diff #449217)

One RUN line for the default code model suffices. There is no codegen optimization difference.

A patch should not introduce disabled tests this way. If a dependent patch introduces the functionality, the test should be added to that patch.

[MaskRay]

llvm/docs/PCSectionsMetadata.rst

28

IIRC metadata is a legacy syntax (llvm 3.5 time). New syntax doesn't use metadata. The doc here should use the new syntax.

[melver]

In D130875#3695416, @MaskRay wrote:

This introduces new LangRef constructs. I suggest that you enlarge the reviewer list.

In D130875#3690485, @melver wrote:

Associated RFC of this series: https://discourse.llvm.org/t/rfc-pc-keyed-metadata-at-runtime/64191

This link can be amended into the summary (commit message).

I thought to wait for some comments on the RFC.
I'll add more reviewers after reshuffling the series.

[melver]

llvm/test/CodeGen/AArch64/pcsections.ll

1 ↗

(On Diff #449217)

I don't understand. Removing any one line here will reduce coverage.

We have RUNs for each -O0 to -O3 with the default code model, and then only O1 with a large code model.

[melver]

llvm/test/CodeGen/AArch64/pcsections.ll

1 ↗

(On Diff #449217)

Note that e.g. on arm64 -O0 uses GlobalISel and -O1 and above use FastISel+SelectionDAG, so using different optimization levels will test different ISels. Different optimization levels have helped me catch bugs in earlier implementations.

[melver]

Address comments.

[MaskRay]

llvm/test/CodeGen/AArch64/pcsections.ll

1 ↗

(On Diff #449217)

To exercise all of GlobalISel FastISel SelectionDAG, the best way is to use explicit options like -fast-isel=, -global-isel=, instead of relying on -O0/-O1/-O2/-O3. -O* runs a large set of codegen pipeline which is pretty irrelevant to this patch.

For a patch series, it's not suitable to add BROKEN: in patch a and change them to RUN: in patch b. This should be refactored to just add RUN: in patch b. If you feel that showding the diff is important, change a to add RUN: about the current state and change b to show the difference.

[vitalybuka]

llvm/lib/IR/MDBuilder.cpp

162

There is TEST_F(MDBuilderTest, which can be updated

166

is implicit mapping from Sections[i] to AuxData[i] in interface is neccecary?
can you do SomeMap<StringRef, <ArrayRef<Constant *>> ?

That would be cleaner

174

why do you need nullptr in that array?

[melver]

llvm/lib/IR/MDBuilder.cpp

166

Map doesn't have stable iteration order. So I'm trying ArrayRef<std::pair<StringRef, SmallVector<Constant*>>>. The problem is that the inner array can't be an ArrayRef, otherwise users of the function have to either convert to something that can convert to ArrayRef<std::pair<StringRef, ArrayRef>> ensuring that whatever inner ref they pass lives long enough.

The reason of the earlier interface was to avoid forcing SmallVector or any particular vector onto callers by sticking to ArrayRef. But by wrapping it into a std::pair, the ArrayRef conversion can't be done implicitly anymore, so usability suffers.

This means that in SanitizerBinaryMetadadata, I'm now using a SmallVector<StringRef, SmallVector<Constant*>>> (instead of just a SmallVector<StringRef>) where the inner SmallVector will always be 0-sized, which wastes space but didn't before. Perhaps this was premature optimization.

174

I don't think we do.

llvm/test/CodeGen/AArch64/pcsections.ll

1 ↗

(On Diff #449217)

In the latest diff of this patch the tests are gone and I moved them to the AsmPrinter patch (now later in the series).

I will additionally add tests to explicitly exercise each ISel there, but want to keep -O* for the additional coverage (whether or not there are irrelevant optimizations being run today, in future this may change).

[vitalybuka]

llvm/docs/PCSectionsMetadata.rst

30

can aux-consts be just an aggregate?

so you have more structured format: section, array, section, array, section, array,....

llvm/lib/IR/MDBuilder.cpp

166

Map doesn't have stable iteration order.

Do you need a particular section order?

pair<stringref, SmallVector> is not nice, but better than obvious mapping.

maybe real builder?

PCSectionsBuilder B =  MDB.getPCSectionsBuilder();
F.setMetadata(
   B.addSection(S, {c1})
      .addSection(S, {}).
      .addSection(S, {c1, c2})
      .buid());

[vitalybuka]

llvm/lib/IR/MDBuilder.cpp

166

Map doesn't have stable iteration order.

Do you need a particular section order?

pair<stringref, SmallVector> is not nice, but better than non-obvious mapping.

maybe real builder?

PCSectionsBuilder B =  MDB.getPCSectionsBuilder();
F.setMetadata(
   B.addSection(S, {c1})
      .addSection(S, {}).
      .addSection(S, {c1, c2})
      .build());

[melver]

• Change MDNode format to take aux constants as another MDNode.
• Change createPCSections interface.
• Add MDBuilderTest test case.

[melver]

llvm/docs/PCSectionsMetadata.rst

30

Yes it can - it's a trade-off. Making it another MDTuple makes interpreting and constructing the MDNode more complex. But it might save space if the same auxiliary data is reused for different sections.

On a whole, placing the auxiliary data into a separate tuple seems nicer, so I've changed the format. However, I feel there's no point in artificially restricting only 1 tuple after a section string, so things like `!0 = !{"sec1", !1, !2, "sec2", ..}` will be allowed (distinguishing MDString and MDNode within the outer tuple is unambiguous).

llvm/lib/IR/MDBuilder.cpp

166

We need order for deterministic compilation and tests. :-/

The main downside of doing pair<StringRef, SmallVector> is a little bit of wasted space. A builder won't help with that and for now just seems more complex. The easiest seems to be to just define MDBuilder::PCSection which users can use as entry type for any type of vector or array.

[vitalybuka]

LGTM. Thanks!

[melver]

Update documentation with "Guarantees on Code Generation".

[melver]

Fix typo.

[melver]

Rebase.

[melver]

Hi all - if there are no more major comments, we intend to land the entire patch series mid-week or early next week.
Thanks!

[vitalybuka]

In D130875#3770686, @melver wrote:

Hi all - if there are no more major comments, we intend to land the entire patch series mid-week or early next week.
Thanks!

I highly recommend do not lend them at once, and have delay in a few hours between patches.
Give a chance to bots maintainers to revert just a single patch when something goes wrong.

[melver]

In D130875#3770826, @vitalybuka wrote:

In D130875#3770686, @melver wrote:

Hi all - if there are no more major comments, we intend to land the entire patch series mid-week or early next week.
Thanks!

I highly recommend do not lend them at once, and have delay in a few hours between patches.
Give a chance to bots maintainers to revert just a single patch when something goes wrong.

Thanks, will do.

llvm/docs/PCSectionsMetadata.rst

+=========================
+LLVM PC Sections Metadata
+=========================
+
+.. contents::
+   :local:
+
+Introduction
+============
+
+PC Sections Metadata can be attached to instructions and functions, for which
+addresses, viz. program counters (PCs), are to be emitted in specially encoded
+binary sections. Metadata is assigned as an ``MDNode`` of the ``MD_pcsections``
+(``!pcsections``) kind; the following section describes the metadata format.
+
+Metadata Format
+===============
+
+An arbitrary number of interleaved ``MDString`` and constant operators can be
+added, where a new ``MDString`` always denotes a section name, followed by an
+arbitrary number of auxiliary constant data encoded along the PC of the
+instruction or function. The first operator must be a ``MDString`` denoting the
+first section.
+
+.. code-block:: none
+
+  !0 = !{
+    !"<section#1>"
+    [ , !1 ... ]
+    [ !"<section#2">
+      [ , !2 ... ]
+      ... ]
+  }
+  !1 = !{ iXX <aux-consts#1>, ... }
+  !2 = !{ iXX <aux-consts#2>, ... }
+  ...
+
+The occurrence of ``section#1``, ``section#2``, ..., ``section#N`` in the
+metadata causes the backend to emit the PC for the associated instruction or
+function to all named sections. For each emitted PC in a section #N, the
+constants ``aux-consts#N`` in the tuple ``!N`` will be emitted after the PC.
+Multiple tuples with constant data may be provided after a section name string
+(e.g. ``!0 = !{"s1", !1, !2}``), and a single constant tuple may be reused for
+different sections (e.g. ``!0 = !{"s1", !1, "s2", !1}``).
+
+Binary Encoding
+===============
+
+*Instructions* result in emitting a single PC, and *functions* result in
+emission of the start of the function and a 32-bit size. This is followed by
+the auxiliary constants that followed the respective section name in the
+``MD_pcsections`` metadata.
+
+To avoid relocations in the final binary, each PC address stored at ``entry``
+is a relative relocation, computed as ``pc - entry``. To decode, a user has to
+compute ``entry + *entry``.
+
+The size of each entry depends on the code model. With large and medium sized
+code models, the entry size matches pointer size. For any smaller code model
+the entry size is just 32 bits.
+
+Guarantees on Code Generation
+=============================
+
+Attaching ``!pcsections`` metadata to LLVM IR instructions *shall not* affect
+optimizations or code generation outside the requested PC sections.
+
+While relying on LLVM IR metadata to request PC sections makes the above
+guarantee relatively trivial, propagation of metadata through the optimization
+and code generation pipeline has the following guarantees.
+
+Metadata Propagation
+--------------------
+
+In general, LLVM *does not make any guarantees* about preserving IR metadata
+(attached to an ``Instruction``) through IR transformations. When using PC
+sections metadata, this guarantee is unchanged, and ``!pcsections`` metadata is
+remains *optional* until lowering to machine IR (MIR).
+
+Note for Code Generation
+------------------------
+
+As with other LLVM IR metadata, there are no requirements for LLVM IR
+transformation passes to preserve ``!pcsections`` metadata, with the following
+exceptions:
+
+    * The ``AtomicExpandPass`` shall preserve ``!pcsections`` metadata
+      according to the below rules 1-4.
+
+When translating LLVM IR to MIR, the ``!pcsections`` metadata shall be copied
+from the source ``Instruction`` to the target ``MachineInstr`` (set with
+``MachineInstr::setPCSections()``). The instruction selectors and MIR
+optimization passes shall preserve PC sections metadata as follows:
+
+    1. Replacements will preserve PC sections metadata of the replaced
+       instruction.
+
+    2. Duplications will preserve PC sections metadata of the copied
+       instruction.
+
+    3. Merging will preserve PC sections metadata of one of the two
+       instructions (no guarantee on which instruction's metadata is used).
+
+    4. Deletions will loose PC sections metadata.
+
+This is similar to debug info, and the ``BuildMI()`` helper provides a
+convenient way to propagate debug info and ``!pcsections`` metadata in the
+``MIMetadata`` bundle.
+
+Note for Metadata Users
+-----------------------
+
+Use cases for ``!pcsections`` metadata should either be fully tolerant to
+missing metadata, or the passes inserting ``!pcsections`` metadata should run
+*after* all LLVM IR optimization passes to preserve the metadata until being
+translated to MIR.

llvm/docs/Reference.rst

.. toctree::
    HowToSetUpLLVMStyleRTTI
    HowToUseAttributes
    InAlloca
    LangRef
    LibFuzzer
    MarkedUpDisassembly
    MIRLangRef
    OptBisect
+   PCSectionsMetadata
    PDB/index
    PointerAuth
    ScudoHardenedAllocator
    MemTagSanitizer
    Security
    SecurityTransparencyReports
    SegmentedStacks
    StackMaps

llvm/include/llvm/IR/FixedMetadataKinds.def

 LLVM_FIXED_MD_KIND(MD_noundef, "noundef", 29)
 LLVM_FIXED_MD_KIND(MD_annotation, "annotation", 30)
 LLVM_FIXED_MD_KIND(MD_nosanitize, "nosanitize", 31)
 LLVM_FIXED_MD_KIND(MD_func_sanitize, "func_sanitize", 32)
 LLVM_FIXED_MD_KIND(MD_exclude, "exclude", 33)
 LLVM_FIXED_MD_KIND(MD_memprof, "memprof", 34)
 LLVM_FIXED_MD_KIND(MD_callsite, "callsite", 35)
 LLVM_FIXED_MD_KIND(MD_kcfi_type, "kcfi_type", 36)
+LLVM_FIXED_MD_KIND(MD_pcsections, "pcsections", 37)

llvm/include/llvm/IR/MDBuilder.h

 // create LLVM metadata with a consistent and simplified interface.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_IR_MDBUILDER_H
 #define LLVM_IR_MDBUILDER_H
 
 #include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/IR/GlobalValue.h"
 #include "llvm/Support/DataTypes.h"
 #include <utility>
 
 namespace llvm {
 
 class APInt;
public:
   /// Merge the new callback encoding \p NewCB into \p ExistingCallbacks.
   MDNode *mergeCallbackEncodings(MDNode *ExistingCallbacks, MDNode *NewCB);
 
   /// Return metadata feeding to the CodeGen about how to generate a function
   /// prologue for the "function" santizier.
   MDNode *createRTTIPointerPrologue(Constant *PrologueSig, Constant *RTTI);
 
   //===------------------------------------------------------------------===//
+  // PC sections metadata.
+  //===------------------------------------------------------------------===//
+
+  /// A pair of PC section name with auxilliary constant data.
+  using PCSection = std::pair<StringRef, SmallVector<Constant *>>;
+
+  /// Return metadata for PC sections.
+  MDNode *createPCSections(ArrayRef<PCSection> Sections);
+
+  //===------------------------------------------------------------------===//
   // AA metadata.
   //===------------------------------------------------------------------===//
 
 protected:
   /// Return metadata appropriate for a AA root node (scope or TBAA).
   /// Each returned node is distinct from all other metadata and will never
   /// be identified (uniqued) with anything else.
   MDNode *createAnonymousAARoot(StringRef Name = StringRef(),

llvm/lib/IR/MDBuilder.cpp

 MDNode *MDBuilder::createRTTIPointerPrologue(Constant *PrologueSig,
                                              Constant *RTTI) {
   SmallVector<Metadata *, 4> Ops;
   Ops.push_back(createConstant(PrologueSig));
   Ops.push_back(createConstant(RTTI));
   return MDNode::get(Context, Ops);
 }
 
+MDNode *MDBuilder::createPCSections(ArrayRef<PCSection> Sections) {
+  SmallVector<Metadata *, 2> Ops;
+
+  for (const auto &Entry : Sections) {
+    const StringRef &Sec = Entry.first;
+    Ops.push_back(createString(Sec));
+
+    // If auxiliary data for this section exists, append it.
+    const SmallVector<Constant *> &AuxConsts = Entry.second;
+    if (!AuxConsts.empty()) {
+      SmallVector<Metadata *, 1> AuxMDs;
+      AuxMDs.reserve(AuxConsts.size());
+      for (Constant *C : AuxConsts)
+        AuxMDs.push_back(createConstant(C));
+      Ops.push_back(MDNode::get(Context, AuxMDs));
+    }
+  }
+
+  return MDNode::get(Context, Ops);
+}
+
 MDNode *MDBuilder::createAnonymousAARoot(StringRef Name, MDNode *Extra) {
   SmallVector<Metadata *, 3> Args(1, nullptr);
   if (Extra)
     Args.push_back(Extra);
   if (!Name.empty())
     Args.push_back(createString(Name));
   MDNode *Root = MDNode::getDistinct(Context, Args);
 

llvm/unittests/IR/MDBuilderTest.cpp

 //===- llvm/unittests/MDBuilderTest.cpp - MDBuilder unit tests ------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/IR/MDBuilder.h"
+#include "llvm/IR/Constants.h"
 #include "llvm/IR/IRBuilder.h"
 #include "llvm/IR/Metadata.h"
 #include "llvm/IR/Operator.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
 
 namespace {
TEST_F(MDBuilderTest, createTBAANode) {
   EXPECT_EQ(cast<MDString>(N2->getOperand(0))->getString(), "Node");
   EXPECT_EQ(N0->getOperand(1), R);
   EXPECT_EQ(N1->getOperand(1), R);
   EXPECT_EQ(N2->getOperand(1), R);
   EXPECT_TRUE(mdconst::hasa<ConstantInt>(N2->getOperand(2)));
   EXPECT_EQ(mdconst::extract<ConstantInt>(N2->getOperand(2))->getZExtValue(),
             1U);
 }
+TEST_F(MDBuilderTest, createPCSections) {
+  MDBuilder MDHelper(Context);
+  ConstantInt *C1 = ConstantInt::get(Context, APInt(8, 1));
+  ConstantInt *C2 = ConstantInt::get(Context, APInt(8, 2));
+  MDNode *PCS = MDHelper.createPCSections({{"s1", {C1, C2}}, {"s2", {}}});
+  ASSERT_EQ(PCS->getNumOperands(), 3U);
+  const auto *S1 = dyn_cast<MDString>(PCS->getOperand(0));
+  const auto *Aux = dyn_cast<MDNode>(PCS->getOperand(1));
+  const auto *S2 = dyn_cast<MDString>(PCS->getOperand(2));
+  ASSERT_NE(S1, nullptr);
+  ASSERT_NE(Aux, nullptr);
+  ASSERT_NE(S2, nullptr);
+  EXPECT_EQ(S1->getString(), "s1");
+  EXPECT_EQ(S2->getString(), "s2");
+  ASSERT_EQ(Aux->getNumOperands(), 2U);
+  ASSERT_TRUE(isa<ConstantAsMetadata>(Aux->getOperand(0)));
+  ASSERT_TRUE(isa<ConstantAsMetadata>(Aux->getOperand(1)));
+  EXPECT_EQ(mdconst::extract<ConstantInt>(Aux->getOperand(0))->getValue(),
+            C1->getValue());
+  EXPECT_EQ(mdconst::extract<ConstantInt>(Aux->getOperand(1))->getValue(),
+            C2->getValue());
 }
+} // namespace