[PATCH] D84473: Dump Accumulator

[James Henderson via Phabricator via cfe-commits]

jhenderson requested changes to this revision.
jhenderson added a comment.
This revision now requires changes to proceed.

I'm assuming you need llvm-readobj to test your changes? In which case, the llvm-readobj change needs to come first, and then you can write tests using it to test this change (at the moment, the testing for this seems insufficient to me). Do you have a patch yet for the llvm-readobj side?

Could you add some documentation somewhere, either in code comments or somewhere else, explaining the output format you actually want this to be? From my casual reading (I'm not the most knowledgeable of people in this area), it looks like you're creating an ELF SHT_NOTE section, but don't use the SHT_NOTE ELF format (see https://www.sco.com/developers/gabi/latest/ch5.pheader.html#note_section for details). I would actually expect this to emit a section with a new section type, so that the consumer (llvm-readobj) can more easily identify it. Alternatively, it could just be a basic SHT_NOTE section, with a specific note type. Under no circumstances should consumers need to check the section header name to identify the section (string comparisons are expensive, and should be avoided where possible - the general intent for ELF is for different kinds of sections to be distinguished by their types and flags).



================
Comment at: llvm/include/llvm/Analysis/DumpAccumulator.h:56
+
+#include <string>
+
----------------
Spurious include?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84473