Page MenuHomePhabricator

[elfabi] Add option to manually specify file read format
ClosedPublic

Authored by amontanez on Dec 12 2018, 2:17 PM.

Details

Summary

Although llvm-elfabi will attempt to read input files without needing the format to be manually specified, doing so has the potential to introduce extraneous errors that can hinder debugging (since multiple readers may fail in attempts to read the file). This change allows the input file format to be manually specified to force elfabi to use a single reader. This makes it easier to test and debug errors specific to a given reader.

Diff Detail

Repository
rL LLVM

Event Timeline

amontanez created this revision.Dec 12 2018, 2:17 PM
This revision is now accepted and ready to land.Dec 12 2018, 2:28 PM
amontanez updated this revision to Diff 177968.Dec 12 2018, 4:09 PM

Small change to read-tbe-as-elf.test to make it easier to debug test failure.

Happy for this to be committed once my comments are addressed.

llvm/test/tools/llvm-elfabi/read-tbe-as-elf.test
16

Nit: I think the style is to have a space between the # and CHECK.

18

Maybe worth checking the filename part of this in this error message.

llvm/test/tools/llvm-elfabi/read-tbe-as-tbe.test
3

Just run FileCheck --input-file=%t, rather than using cat.

Not sure if this available here, but something worth considering adding if it isn't. A lot of tests put a file on stdout using '-' as the filename. Doing so would then allow you to run FileCheck on it without needing to call cat or use --input-file:
# RUN: llvm-elfabi -TBE %s -emit-tbe=- | FileCheck %s

llvm/tools/llvm-elfabi/llvm-elfabi.cpp
41

I take it there's no way of making the input argument match-up case insensitive? I know that options can be case-insensitive if the command line library is used appropriately.

amontanez updated this revision to Diff 178989.Dec 19 2018, 4:10 PM
amontanez marked 3 inline comments as done.
  • Rebase on changes to parent patch.
  • Fix test formatting.
  • Input format flags are now lowercase only.
amontanez marked 2 inline comments as done.Dec 19 2018, 4:19 PM
amontanez added inline comments.
llvm/tools/llvm-elfabi/llvm-elfabi.cpp
41

I looked into this for a while and couldn't find any way make these case insensitive. I assumed it would be possible too. I realized that the other command line options weren't case insensitive either, so I've set these to be lowercase only for consistency.

jhenderson accepted this revision.Jan 2 2019, 4:23 AM

LGTM.

llvm/tools/llvm-elfabi/llvm-elfabi.cpp
41

@jakehehrlich, didn't you run into something like this in llvm-objcopy? Maybe you could shed some light on things.