[yaml2obj] Add -D k=v to preprocess the input YAML

[MaskRay]

Summary

Examples:

yaml2obj -D MACHINE=EM_386 a.yaml -o a.o
yaml2obj -D MACHINE=0x1234 a.yaml -o a.o

where a.yaml contains:

--- !ELF
FileHeader:
  Class:   ELFCLASS64
  Data:    ELFDATA2MSB
  Type:    ET_REL
  Machine: [[MACHINE]]

[MaskRay]

drop a redundant test

[merge_guards_bot]

Unit tests: fail. 62372 tests passed, 2 failed and 839 were skipped.

failed: LLVM.tools/yaml2obj/ELF/unused-overridden.yaml
failed: libc++.std/thread/thread_mutex/thread_mutex_requirements/thread_mutex_requirements_mutex/thread_mutex_class/try_lock.pass.cpp

clang-tidy: fail. clang-tidy found 0 errors and 5 warnings. 0 of them are added as review comments below (why?).

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[merge_guards_bot]

Unit tests: pass. 62374 tests passed, 0 failed and 839 were skipped.

clang-tidy: fail. clang-tidy found 0 errors and 5 warnings. 0 of them are added as review comments below (why?).

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[MaskRay]

Improve tests

[merge_guards_bot]

Unit tests: fail. 62379 tests passed, 1 failed and 839 were skipped.

failed: libc++.std/thread/thread_mutex/thread_mutex_requirements/thread_mutex_requirements_mutex/thread_mutex_recursive/try_lock.pass.cpp

clang-tidy: fail. clang-tidy found 0 errors and 5 warnings. 0 of them are added as review comments below (why?).

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[grimar]

Sorry, but I'd probably prefer to have a different approach.
Something that works like a preprocessor. We could have:

--- !ELF
FileHeader:
  [[FOO]]:   ELFCLASS32
  Data:    ELFDATA2LSB
  Type:    ET_REL
  Machine: [[MACHINE]]

And then have a code that replaces `[[MACHINE]]', '[[FOO]]' strings in the input data
with something that is specified with -D option. e.g.

yaml2obj -DMACHINE="XXX" -DFOO="Class"

gives

--- !ELF
FileHeader:
  Class:   ELFCLASS32
  Data:    ELFDATA2LSB
  Type:    ET_REL
  Machine: XXX

It is much more generic (can be used for all targets, fields, etc), and it should be much simpler to implement.
As a bonus we will be able to modify any part of the YAML. It also makes inputs to be more clear,
because currently it is not obvious that some part is overridable.

And I do not have any concerns about the fact we modify the input YAML description here,
because the usage of such construction is limited to our test cases
and we should not met issues with it. When there is no -D option given, no preprocessing should happen. Should be safe enough.

[jhenderson]

I mostly agree with @grimar, although I don't think it's necessary to support arbitrary Key names, only Value names. Should the proposal (in whatever form it takes) go up on the mailing lists as an RFC? It feels like a useful feature that more people might want to talk about.

[MaskRay]

Repurpose.

Adopt grimar's suggestion

[merge_guards_bot]

Unit tests: fail. 62422 tests passed, 1 failed and 845 were skipped.

failed: LLVM.Analysis/ConstantFolding/vscale-shufflevector.ll

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[grimar]

I am going to debug this. My first impression is that the direction is right,
but it needs a bit of polishing.
Will add more comments soon.

llvm/test/tools/yaml2obj/ELF/emachine.yaml

1 ↗

(On Diff #242158)

This is a refactoring change, right? I'd leave such changes for a follow-up patch(es).

llvm/test/tools/yaml2obj/preprocess.yaml

10 ↗

(On Diff #242158)

It is probably usefull to add cases with more than one -D:

-D FOO=x -D BAR=y

-D FOO=x -D FOO=y

llvm/tools/yaml2obj/yaml2obj.cpp

35

Seems the description should start from the upper case letter and perhaps I'd reword it somehow.
It is not a list it seems? It is a single redefinition as far I understand.

54

Perhaps Substitutions or may be just Defines? Up to you,
but -D option is closer to the latter word I think.

56

What is F stands for? I`d rename to Name and Value..

[jhenderson]

llvm/test/tools/yaml2obj/preprocess.yaml

1 ↗

(On Diff #242158)

Perhaps also worth a test case that "-DNAME=..." is accepted, since that matches the compiler syntax, so would be a likely usage.

22 ↗

(On Diff #242158)

I'm not sure this comment line adds anything. My immediate reaction was "what's this test got to do with FileCheck?"

llvm/tools/yaml2obj/yaml2obj.cpp

35

I'd change this help text to "Defined the specified macros to their specified definition. The syntax is <macro>=<definition>".

56

Let's use Macro and Definition

The "field/value" naming doesn't really make sense since the new functionality is much more general purpose.

59

This error should a) mention the switch name, and b) probably not use the word "override" since nothing is being overridden as such. How about "invalid syntax for -D: xxx" (where xxx is the input string). If you think you need extra context, I'd split "missing '=' from "missing macro name".

69

I != StringRef::npos

[jhenderson]

(@grimar got in with comments as I was writing mine, so sorry for any duplication!)

[grimar]

llvm/test/tools/yaml2obj/preprocess.yaml

26 ↗

(On Diff #242158)

This does not work for me under windows.

echo -e 'a[[\n[[a\n[[a]\n[[a][[a]][[a]]' | cmp - %t.nosubst

I see the following when I run this line:

D:\Work3\LLVM\llvm-project\build\test\tools\yaml2obj\Output>echo -e 'a[[\n[[a\n[
[a]\n[[a][[a]][[a]]' | cmp - preprocess.yaml.tmp.nosubst
- preprocess.yaml.tmp.nosubst различаются: байт 1, строка 1

What means that byte 1 at the line 1 is different from the source. (My apologies, I do not know how to
switch to displaying errors in English for cmp).

I did not dig deeper because anyways I do not like this way of checking symbol names (sorry :),
so I'd ask you to rewrite in a more traditional way.

llvm/tools/yaml2obj/yaml2obj.cpp

62

Substitution[F] = V;

try_emplace is not needed to be used here, is't? StringRef is a cheap type for copying and
moving concept does not make sense at all I think.

84

I'd suggest something like the following

std::vector<std::pair<std::string, StringRef>> Substitution;
  for (StringRef Define : D) {
    StringRef F, V;
    std::tie(F, V) = Define.split('=');
    if (!Define.count('=') || F.empty()) {
      ErrHandler("bad override, missing field name: " + Define);
      return {};
    }
    Substitution.push_back({(Twine("[[") + F + "]]").str(), V});
  }

  std::string Document = Buf.str();
  if (Substitution.empty())
    return Document;

  for (std::pair<std::string, StringRef> &P : Substitution) {
    while (true) {
      auto I = Document.find(P.first);
      if (I == std::string::npos)
        break;
      Document = Document.replace(I, P.first.size(), P.second.str());
    }
  }

[MaskRay]

Address review comments.

llvm/test/tools/yaml2obj/preprocess.yaml

26 ↗

(On Diff #242158)

Switched to

echo -e '.........' > ...
diff -u ... %t.nosubst

Unfortunately we cannot use FileCheck because FileCheck does not like [[ which is not used as a FileCheck pattern.

llvm/tools/yaml2obj/yaml2obj.cpp

84

Maybe we should avoid quadratic complexity behavior, just in case that there are a long run of [[[[[[[[[[.

[merge_guards_bot]

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[MaskRay]

Simplify with Buf.find_first_of

(OT: StringRef::find_first_of is O(n * 256) but the method can be imroved.)

[MaskRay]

Simplify...

[merge_guards_bot]

Unit tests: unknown.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[merge_guards_bot]

Unit tests: unknown.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[grimar]

llvm/test/tools/yaml2obj/preprocess.yaml

26 ↗

(On Diff #242158)

Unfortunately we cannot use FileCheck because FileCheck does not like [[ which is not used as a FileCheck pattern.

Ah, I see. I did not know that. That is unfortunate behavior.

llvm/tools/yaml2obj/yaml2obj.cpp

84

just in case that there are a long run of [[[[[[[[[[.

It reminded me about D36307. I believe that [[[[[[ is a misuse of the feature and hence we probably should not care much.
The approach I suggested is just very simple and it is much easier to read. I think the common use case for this feature will be
"replace 1-2 defines in a short enough YAML", i.e. I am not sure we should think about an additional optimizations here.

Probably it is time to hear a third opinion though. @jhenderson, what do you think?

[MaskRay]

llvm/tools/yaml2obj/yaml2obj.cpp

84

Doing replace repeatedly may also have a non-convergence problem...

[jhenderson]

llvm/tools/yaml2obj/yaml2obj.cpp

84

Recursive replacement causes issues. For example -DFOO=[[FOO]] would result in an infinite loop. Convergence issues are only a problem if we try to do this. I personally don't think we should in the first version.

Another issue is what to do about variables that expand to other variables. One example is -D VAR1=FOO -D VAR2=[[VAR1]]BAR which will result in [[VAR2]] becoming [[VAR1]]BAR if specified in that order, or FOOBAR if VAR2 is defined first. I don't think we want this behaviour, as it is unstable and unobvious.

(Relatedly, I think we should have a test showing that variables that expand to a pattern that could be another variable aren't recursively expanded)

As for the quadratic complexity, I'm not too bothered by it in this case, as I doubt we'll ever get a YAML doc in a test where it really matters (and even if we do, there won't be many, so the overall slow-down in the testsuite will be minimal).

FWIW, I think we can probably forbid names containing '[' or ']' themselves.

[MaskRay]

Improve a test

[MaskRay]

llvm/tools/yaml2obj/yaml2obj.cpp

84

Added the following to macro.yaml

# RUN: yaml2obj --docnum=2 -D a0='[[a1]]' -D a1='[[a0]]' %s | llvm-nm --just-symbol-name --no-sort - > %t.recur
# RUN: echo -e '[[a1]]\n[[a0]]' > %t0.recur
# RUN: diff -u %t0.recur %t.recur

This patch avoids quadratic complexity and non-convergence with very little code, so I think sticking with the current version is fine.

[merge_guards_bot]

Unit tests: fail. 62504 tests passed, 8 failed and 844 were skipped.

failed: LLVM.tools/yaml2obj/help.test
failed: lld.ELF/compressed-input-alignment.test
failed: lld.ELF/invalid/bad-reloc-target.test
failed: lld.ELF/invalid/common-symbol-alignment.test
failed: lld.ELF/invalid/dynamic-section-broken.test
failed: lld.ELF/invalid/symtab-sh-info.s
failed: lld.ELF/mips-elf-flags-err.test
failed: lld.ELF/mips-fp-flags-err.test

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[grimar]

OK. I have 2 non-strong comments/suggestions about the current code.

llvm/tools/yaml2obj/yaml2obj.cpp

54

Should we exit early for a general case?

if (D.empty())
  return Buf.str();

It might simplify things for those who want to debug the code.

74

Here is a little logical issue. If we have input like "[[AAA[",
then you still assign "AAA" to Macro and perform a lookup,
but at fact "AAA" is not a macro and you do not need to do a lookup for it.

[MaskRay]

llvm/tools/yaml2obj/yaml2obj.cpp

54

When an early exit is just for the fast path (for the common case), I prefer not to add them.

For example,

if (thing.empty())
  return;
for (... : thing) {
  do something
}

I think if (thing.empty()) is unnecessary, unless the code is a critical path and the early return thing has significant performance improvement.

74

Yes. The if can be split to avoid a lookup. It will add two lines. I can do that.

[grimar]

llvm/tools/yaml2obj/yaml2obj.cpp

54

I see what you mean, though here the situation is a bit different probably: too many code is involved, i.e. it is not a trivial loop from your example. it is somehting much more. I do not insist here though, so up to you.

74

I am not concerned about the perfomance at all here, but I'd do that for code readability as it is a bit too noticable and IMO might look like a possible issue for a new readers.

[MaskRay]

Split an if

[grimar]

LGTM, thanks! Please wait for @jhenderson too.

[jhenderson]

LGTM too!

llvm/test/tools/yaml2obj/macro.yaml

+# RUN: yaml2obj -D NAME= %s | llvm-nm - | FileCheck --check-prefix=FBAR %s
+# FBAR: U fbar
+
+# RUN: yaml2obj -D NAME=o %s | llvm-nm - | FileCheck --check-prefix=FOOBAR %s
+# RUN: yaml2obj -DNAME=o %s | llvm-nm - | FileCheck --check-prefix=FOOBAR %s
+# FOOBAR: U foobar
+
+# RUN: not yaml2obj -D NAME %s 2>&1 | FileCheck --check-prefix=ERR1 %s
+# ERR1: error: invalid syntax for -D: NAME
+
+# RUN: not yaml2obj -D =value %s 2>&1 | FileCheck --check-prefix=ERR2 %s
+# ERR2: error: invalid syntax for -D: =value
+
+# RUN: not yaml2obj -D NAME=a -D NAME=b %s 2>&1 | FileCheck --check-prefix=ERR3 %s
+# ERR3: error: 'NAME' redefined
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Symbols:
+  - Name: f[[NAME]][[NAME]]bar
+
+## Digits can appear in macro names. Test that we don't expand macros recursively.
+# RUN: yaml2obj --docnum=2 -D a0='[[a1]]' -D a1='[[a0]]' %s | llvm-nm --just-symbol-name --no-sort - > %t.recur
+# RUN: echo -e '[[a1]]\n[[a0]]' > %t0.recur
+# RUN: diff -u %t0.recur %t.recur
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Symbols:
+  - Name: "[[a0]]"
+  - Name: "[[a1]]"
+
+## Test unterminated [[.
+# RUN: yaml2obj --docnum=3 %s | llvm-nm --just-symbol-name --no-sort - > %t.nosubst
+# RUN: echo -e 'a[[\n[[a]\n[[a[[a]]\n[[a][[a]][[b]]' > %t0.nosubst
+# RUN: diff -u %t0.nosubst %t.nosubst
+
+# RUN: yaml2obj --docnum=3 -D a=b -D b=c %s | llvm-nm --just-symbol-name --no-sort - > %t.subst
+# RUN: echo -e 'a[[\n[[a]\n[[ab\n[[a]bc' > %t0.subst
+# RUN: diff -u %t0.subst %t.subst
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Symbols:
+  - Name: "a[["
+  - Name: "[[a]"
+  - Name: "[[a[[a]]"
+  - Name: "[[a][[a]][[b]]"

llvm/tools/yaml2obj/yaml2obj.cpp

 #include <system_error>
 
 using namespace llvm;
 
 namespace {
 cl::OptionCategory Cat("yaml2obj Options");
 
 cl::opt<std::string> Input(cl::Positional, cl::desc("<input file>"),
                            cl::init("-"), cl::cat(Cat));
 
+cl::list<std::string>
+    D("D", cl::Prefix,
+      cl::desc("Defined the specified macros to their specified "
+               "definition. The syntax is <macro>=<definition>"));
+
 cl::opt<unsigned>
     DocNum("docnum", cl::init(1),
            cl::desc("Read specified document from input (default = 1)"),
            cl::cat(Cat));
 
 cl::opt<std::string> OutputFilename("o", cl::desc("Output filename"),
                                     cl::value_desc("filename"), cl::init("-"),
                                     cl::Prefix, cl::cat(Cat));
 } // namespace
 
+static Optional<std::string> preprocess(StringRef Buf,
+                                        yaml::ErrorHandler ErrHandler) {
+  DenseMap<StringRef, StringRef> Defines;
+  for (StringRef Define : D) {
+    StringRef Macro, Definition;
+    std::tie(Macro, Definition) = Define.split('=');
+    if (!Define.count('=') || Macro.empty()) {
+      ErrHandler("invalid syntax for -D: " + Define);
+      return {};
+    }
+    if (!Defines.try_emplace(Macro, Definition).second) {
+      ErrHandler("'" + Macro + "'" + " redefined");
+      return {};
+    }
+  }
+
+  std::string Preprocessed;
+  while (!Buf.empty()) {
+    if (Buf.startswith("[[")) {
+      size_t I = Buf.find_first_of("[]", 2);
+      StringRef Macro = Buf.substr(2, I - 2);
+      auto It = Defines.find(Macro);
+      if (It != Defines.end() && Buf.substr(I).startswith("]]")) {
+        Preprocessed += It->second;
+        Buf = Buf.substr(I + 2);
+        continue;
+      }
+    }
+
+    Preprocessed += Buf[0];
+    Buf = Buf.substr(1);
+  }
+
+  return Preprocessed;
+}
+
 int main(int argc, char **argv) {
   InitLLVM X(argc, argv);
   cl::HideUnrelatedOptions(Cat);
   cl::ParseCommandLineOptions(
       argc, argv, "Create an object file from a YAML description", nullptr,
       nullptr, /*LongOptionsUseDoubleDash=*/true);
 
   auto ErrHandler = [](const Twine &Msg) {
     WithColor::error(errs(), "yaml2obj") << Msg << "\n";
   };
 
   std::error_code EC;
   std::unique_ptr<ToolOutputFile> Out(
       new ToolOutputFile(OutputFilename, EC, sys::fs::OF_None));
   if (EC) {
     ErrHandler("failed to open '" + OutputFilename + "': " + EC.message());
     return 1;
   }
 
   ErrorOr<std::unique_ptr<MemoryBuffer>> Buf =
       MemoryBuffer::getFileOrSTDIN(Input);
   if (!Buf)
     return 1;
 
-  yaml::Input YIn(Buf.get()->getBuffer());
+  Optional<std::string> Buffer = preprocess(Buf.get()->getBuffer(), ErrHandler);
+  if (!Buffer)
+    return 1;
+  yaml::Input YIn(*Buffer);
   if (!convertYAML(YIn, Out->os(), ErrHandler, DocNum))
     return 1;
 
   Out->keep();
   Out->os().flush();
   return 0;
 }