[PATCH] D153376: Introducing llvm-cm: A Cost Model Tool

[Dayann D'almeida via Phabricator via llvm-commits]

JestrTulip added inline comments.


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:117
+    WithColor::warning(errs(), "llvm-cm")
+        << "Failed to get section name: " << toString(SecNameOrErr.takeError())
+        << "\n";
----------------
jhenderson wrote:
> JestrTulip wrote:
> > jhenderson wrote:
> > > https://llvm.org/docs/CodingStandards.html#error-and-warning-messages
> > > 
> > > Also, test case?
> > Regarding the test cases for aspects such as Section Name, a good portion of the error handling for disassembly was modeled off the error handling in tools such as objdump and mca. For this specific case, and many of those that are unaddressed within the program, I find it hard to come up with a way to properly test these occurrences. 
> > For example, this error requires a Section to exist within the file, but its name to not be found. 
> > If I could receive any ideas on how to properly test situations like this, i'd be very grateful. 
> > 
> > 
> Take a look at the test cases for llvm-readobj - there are many examples where it uses yaml2obj to customise the input in some way, e.g. creating an input without a section header string table (see https://github.com/llvm/llvm-project/blob/main/llvm/test/tools/llvm-readobj/ELF/sections-no-section-header-string-table.test). You should be able to use yaml2obj to achieve a broken input in some manner using these examples (more can be found in the ELF yaml2obj tests too, if you want to explore the various options you have). NB: you don't need to test every possible failure mode that the underlying library can hit (these should already have testing elsewhere). Instead, you should make sure you have testing that covers the case where a particularly library function returns an error, to show that you handled the error correctly.
I do have a question regarding some of the tests. I've used yaml2obj to for some of them, but I was wondering how to create the necessary conditions for others, namely the error messages that occur despite a proper target (e.g. AsmInfo, FeatureVals, SubInfo, etc..). I was also wondering if there was a way to generate an object file with invalid sections, for this message. 

```
if (!SecNameOrErr) {
    WithColor::warning(errs(), "llvm-cm")
        << "Failed to get section name: " << toString(SecNameOrErr.takeError())
        << "\n";
}
```


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:75
+class ExitIf {
+   public:
+    ExitIf(bool Cond) : Condition(Cond) {}
----------------
jhenderson wrote:
> JestrTulip wrote:
> > jhenderson wrote:
> > > jhenderson wrote:
> > > > Have you run clang-format on your new code? Because this doesn't look formatted according to the standard format.
> > > Ping - not addressed.
> > I've run git clang-format on the most recent patch.  Just to make sure I'm doing it correctly: 
> > I first git added all the modified files, 
> > I then ran git-clang format from the root of my repo 
> > I then received the message "clang-format did not modify any files" 
> >  
> I personally don't use git-clang-format to do it - I use a clang-format tool that comes along with my Visual Studio setup, so I can't comment. Perhaps @MaskRay or another review could assist, because this is definitely not correctly formatted.
@MaskRay suggested 
```
git diff -U0 --no-color --relative 'HEAD^' -- | llvm-project/clang/tools/clang-format/clang-format-diff.py -p1 -i
```
so I used that. 


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:164
+
+// TODO: Share this with llvm-objdump.cpp. 
+static uint8_t getElfSymbolType(const llvm::object::ObjectFile &Obj, const llvm::object::SymbolRef &Sym) {
----------------
jhenderson wrote:
> JestrTulip wrote:
> > jhenderson wrote:
> > > Any reason you can't do this upfront, like you did with the section filter stuff?
> > I was planning on including these changes in a later patch, since the overall scope of this one is so minimal, I wanted to avoid changing many other files. 
> This is brand new code. You should really avoid duplicating code in new code, if at all possible. In other words, you should be refactoring the code you want to use in earlier patches, to make it shareable, and then base this patch on top of them.
Regarding the code duplication, where should these shared functions be moved to. I was thinking about ObjectFile.cpp, but I am open to any suggestions. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153376