[PATCH] D30156: llvm-mc-fuzzer: add support for assembly

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added a comment.

> Currently we just attempt assembly and ignore the result.

Ignoring the result is the right thing to do since failure to assemble is an expected response to some inputs. Whether it's a correct response to a particular input can be found by separately running the corpus through the assembler and comparing against a reference (most likely, another assembler).



================
Comment at: tools/llvm-mc-fuzzer/llvm-mc-fuzzer.cpp:86
+       LLVMFuzzerInputBuffer(const uint8_t *data_, size_t size_)
+       : data(reinterpret_cast<const char *>(data_)), size(size_) {
+           init(data, data+size, false);
----------------
reinterpret_cast tends to cause portability problems but this one is ok for the targets LLVM supports in-tree as far as I know.


================
Comment at: tools/llvm-mc-fuzzer/llvm-mc-fuzzer.cpp:133-135
+    static const bool ShowInst = false;
+    static const bool asmverbose = false;
+    static const bool useDwarfDirectory = true;
----------------
These should be 'AsmVerbose' and 'UseDwarfDirectory'

Also, why static? (and similarly for the other trivial constants below)


================
Comment at: tools/llvm-mc-fuzzer/llvm-mc-fuzzer.cpp:137-139
+    if (TripleName.empty())
+        TripleName = sys::getDefaultTargetTriple();
+    static Triple TheTriple(Triple::normalize(TripleName));
----------------
If we're going to do this for the assembler, we should do it for the disassembler too. It would be strange to be inconsistent between the two


================
Comment at: tools/llvm-mc-fuzzer/llvm-mc-fuzzer.cpp:154
+    const Target *TheTarget = TargetRegistry::lookupTarget(ArchName, TheTriple,
+                                                         Error);
+    if (!TheTarget) {
----------------
Indentation


================
Comment at: tools/llvm-mc-fuzzer/llvm-mc-fuzzer.cpp:179
+    MCInstPrinter *IP = TheTarget->createMCInstPrinter(Triple(TripleName), OutputAsmVariant,
+                                        *MAI, *MCII, *MRI);
+    if (!IP) {
----------------
Indentation


================
Comment at: tools/llvm-mc-fuzzer/llvm-mc-fuzzer.cpp:203-207
+    // TODO: TheTarget->createNullStreamer(Ctx) is an option but
+    //        not yet supported by all of the backends
+    Str.reset(TheTarget->createAsmStreamer(
+                Ctx,  std::move(FOut), asmverbose,
+                useDwarfDirectory, IP, CE, MAB, ShowInst));
----------------
You'll also catch more bugs by running it through the assembly streamer since some things can only be detected during emission.

Optional: Could you make it an option to run it through the object streamer? Ideally, we'd have the -filetype option from llvm-mc. Mips in particular has some things that will only be detected when macro/directive expansion occurs in the object streamer.


================
Comment at: tools/llvm-mc-fuzzer/llvm-mc-fuzzer.cpp:209
+    const int Res = AssembleInput(ProgName, TheTarget, SrcMgr, Ctx, *Str, *MAI, *STI,
+            *MCII, MCOptions);
+
----------------
Indentation


================
Comment at: tools/llvm-mc-fuzzer/llvm-mc-fuzzer.cpp:244
   if (Action == AC_Assemble)
-    errs() << "error: -assemble is not implemented\n";
+    return AssembleOneInput(Data, Size);
   else if (Action == AC_Disassemble)
----------------
kcc wrote:
> I strongly suggest to make this a separate fuzz target instead of using flags. 
> Otherwise it'll be harder to automate running this target. 
I'm not sure what you mean here. What difficulties are you thinking of?

FWIW, this is in line with my original intent which was to mimic llvm-mc's interface.


Repository:
  rL LLVM

https://reviews.llvm.org/D30156