Page MenuHomePhabricator

[llvm-strip] Error when using stdin twice
ClosedPublic

Authored by abrachet on Jun 10 2019, 11:22 PM.

Details

Summary

Implements bug 42204. llvm-strip now warns when the same input file is used more than once, and errors when stdin is used more than once.

Diff Detail

Repository
rL LLVM

Event Timeline

abrachet created this revision.Jun 10 2019, 11:22 PM
abrachet marked 3 inline comments as done.Jun 10 2019, 11:27 PM
abrachet added inline comments.
llvm/test/tools/llvm-objcopy/ELF/same-file-strip.test
6 ↗(On Diff #203980)

I cp to file in the current directory because doing '%p/Inputs/dynrel.elf' on line 9 did not expand the %p.

llvm/tools/llvm-objcopy/CopyConfig.cpp
805 ↗(On Diff #203980)

I don't know why this is the case but the compiler was not content with just StringSet and asked if I meant StringRef. But adding the <> fixed this.

llvm/tools/llvm-objcopy/llvm-objcopy.cpp
88 ↗(On Diff #203980)

The objcopy::error functions explicitly flush errs, I don't know why though because it appears to be unbuffered, but I figured I would follow how the rest of the code does it.

jhenderson added inline comments.Jun 11 2019, 2:53 AM
llvm/test/tools/llvm-objcopy/ELF/same-file-strip.test
1 ↗(On Diff #203980)

A comment at the start of the test briefly explaining its purpose would be useful.

Rather than relying on a pre-canned binary throughout this test, use a temporary object created by yaml2obj, as we want to get rid of all such objects if possible. Additionally, with llvm-strip tests in particular NEVER used pre-canned objects without copying them to a temporary location first, because llvm-strip modifies in place and could corrupt your test input in some way.

4 ↗(On Diff #203980)

I find it a little easier to read if there's a blank line between RUN lines and CHECK lines

6 ↗(On Diff #203980)

'%p' is a lit substitution for RUN lines only, whereas line 9 is a piece of text consumed by FileCheck, which doesn't know about lit substitutions. Instead, you can use FileCheck's -D option to define a variable that you can search for. See https://llvm.org/docs/CommandGuide/FileCheck.html#cmdoption-d-var. This then changes your FileCheck command and WARN check line to something like:

# RUN: ... | FileCheck -check-prefix=WARN %s -DFILE=%p/Inputs/dynrel.elf
# WARN: warning: '[[FILE]]' was already specified

You shouldn't copy a file to an unknown directory, because it's possible that another test is doing the same thing resulting in race conditions and flaky tests. If you ever do need to copy a file, copy it to a path involving %t.

10 ↗(On Diff #203980)

FWIW, you don't need this line anyway, because it will still be created somewhere in the output directory.

llvm/tools/llvm-objcopy/CopyConfig.cpp
809 ↗(On Diff #203980)

Looks like you haven't run clang-format here.

810 ↗(On Diff #203980)

As you are planning on librarifying llvm-objcopy, be VERY wary about adding warnings directly in the code that might be eventually used in the library. If you haven't already, take a look at my lightning talk from last year's US Developers' meeting, which includes some tips and gotchas on this sort of thing:

https://www.youtube.com/watch?v=YSEY4pg1YB0

rupprecht added inline comments.Jun 11 2019, 2:04 PM
llvm/tools/llvm-objcopy/CopyConfig.cpp
805 ↗(On Diff #203980)

What about using a StringMap to track the actual counts so you only report the first time a file is included multiple times?

StringMap<int> InputFileMap;
for (StringRef Filename : Positional)
  if (InputFileMap[Filename]++ == 1) {
    if (Filename == "-") return createStringError(...);
    reportWarning(...); // Only reported the first time we see a dupe file
  }
807 ↗(On Diff #203980)

llvm::is_contained(InputFiles, Filename)

810 ↗(On Diff #203980)

+1, if possible we should not be calling back into llvm-objcopy.cpp

abrachet updated this revision to Diff 204178.Jun 11 2019, 3:06 PM

Update test case. Only warn the first time a file is recognized again.

abrachet marked 11 inline comments as done.Jun 11 2019, 3:15 PM
abrachet added inline comments.
llvm/test/tools/llvm-objcopy/ELF/same-file-strip.test
6 ↗(On Diff #203980)

Very helpful, thank you!

llvm/tools/llvm-objcopy/CopyConfig.cpp
805 ↗(On Diff #203980)

Thanks!

810 ↗(On Diff #203980)

@jhenderson How would you recommend dealing with warnings in this case? I definitely agree with your sentiment and think failing at the local site is generally bad. Not sure how to deal with warnings though, llvm::Error is pretty binary it seems to me, either error or success.

abrachet marked 2 inline comments as done.Jun 11 2019, 3:23 PM
abrachet updated this revision to Diff 204182.Jun 11 2019, 3:25 PM

Added period at the end of comment.

jhenderson added inline comments.Jun 12 2019, 1:52 AM
llvm/test/tools/llvm-objcopy/ELF/same-file-strip.test
1 ↗(On Diff #204182)

strip -> llvm-strip

and add a brief explanation of what is expected.

17–20 ↗(On Diff #204182)

Nit: remove the extra whitespace here and below so that each block has a lined-up column as far to the left as possible, i.e:

Class:   ELFCLASS64
Data:    ELFDATA2LSB
Type:    ET_REL
Machine: EM_X86_64
llvm/tools/llvm-objcopy/CopyConfig.cpp
810 ↗(On Diff #203980)

The exact method for warnings probably depends on what you are planning on doing. In this particular case, I'd recommend probably passing in some form of handler (e.g. a callback function) which is defined by the caller. It would take a string (or possibly an llvm::Error) and report the warning in a manner applicable to the specific application. I'd then leave reportWarning in the original location and pass it in.

If you went the llvm::Error route, you could have the callback return an llvm::Error containing any unhandled issues, which is checked in this code to see if it should continue or not.

rupprecht added inline comments.Jun 12 2019, 2:08 PM
llvm/tools/llvm-objcopy/CopyConfig.cpp
810 ↗(On Diff #203980)

+1 to a callback that logs an Error, and use createFileError instead of manually crafting the filename into the error string

The callback mechanism described in the talk can be seen here: https://github.com/llvm/llvm-project/blob/master/llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp#L837

llvm/tools/llvm-objcopy/llvm-objcopy.cpp
88 ↗(On Diff #203980)

yep, stderr is unbuffered and doesn't require flushing -- it could be removed from there too

abrachet updated this revision to Diff 204417.Jun 12 2019, 9:22 PM

Report warnings through a callback instead of locally.

abrachet marked 7 inline comments as done.Jun 12 2019, 9:24 PM
abrachet added inline comments.
llvm/tools/llvm-objcopy/llvm-objcopy.cpp
57 ↗(On Diff #204417)

I removed this in this patch because Jordan said it could be removed. Is it okay to sneak this one in this patch or should I separate this?

abrachet updated this revision to Diff 204431.Jun 12 2019, 11:11 PM

Switched from passing createFileError to createStringError to the callback. The string created from from toString(Error) by an Error created from createFileError includes the corresponding string of the error_code. But createStringError omits it. Now when warning about the same file twice, the warning will be "warning: '[[FILE]]' was already specified" not "warning: '[[FILE]]' was already specified: Invalid argument"

jhenderson added inline comments.Jun 13 2019, 1:47 AM
llvm/test/tools/llvm-objcopy/ELF/same-file-strip.test
24 ↗(On Diff #204431)

Not that it matters, but I'd usually line up each block independently (i.e. I think you could remove an extra space from everything inside "Sections").

llvm/tools/llvm-objcopy/CopyConfig.h
194 ↗(On Diff #204431)

I'd probably change the name to just "ErrorCallback", and put a note in the comment explaining what this function is supposed to do. Since it's recoverable, and the program might be able to continue, I think it makes sense to return Error, and let the parseStripOptions code check the return value to decide if it is safe to continue or not.

llvm/tools/llvm-objcopy/llvm-objcopy.cpp
57 ↗(On Diff #204417)

Whilst I'd probably do it, strictly it should probably be in its own micro-patch that doesn't need reviewing (as long as you build and run the tests).

abrachet updated this revision to Diff 204689.Jun 13 2019, 8:10 PM

Propagate error values returned from recoverable error callback to parseStripOptions

abrachet marked 5 inline comments as done.Jun 13 2019, 8:13 PM
abrachet added inline comments.
llvm/test/tools/llvm-objcopy/ELF/same-file-strip.test
24 ↗(On Diff #204431)

Wasn't quite sure which white space you were referring to so I copied this from one of your tests.

llvm/tools/llvm-objcopy/CopyConfig.h
194 ↗(On Diff #204431)

How does this look? It is used down on line 816 of CopyConfig.cpp

llvm/tools/llvm-objcopy/llvm-objcopy.cpp
57 ↗(On Diff #204417)

You're right. I put it back.

abrachet marked 2 inline comments as done.Jun 13 2019, 8:17 PM

Looks good, aside from the comment.

llvm/tools/llvm-objcopy/CopyConfig.cpp
718–719 ↗(On Diff #204689)

Maybe rephrase this slightly:

"handle recoverable errors. An Error returned by the callback aborts the parsing and is then returned by this function"

I'd also move the ErrorCallback description up to its declaration.

abrachet updated this revision to Diff 204898.Jun 14 2019, 10:20 PM

Changed comment explaining warning callback.

abrachet marked 2 inline comments as done.Jun 14 2019, 10:21 PM
jhenderson accepted this revision.Jun 17 2019, 9:25 AM

LGTM, aside from one comment.

llvm/tools/llvm-objcopy/CopyConfig.cpp
715 ↗(On Diff #204898)

This comment is identical to its declaration, which seems wrong. Could it just be deleted entirely?

This revision is now accepted and ready to land.Jun 17 2019, 9:25 AM
abrachet marked an inline comment as done.Jun 17 2019, 9:39 AM
abrachet added inline comments.
llvm/tools/llvm-objcopy/CopyConfig.cpp
715 ↗(On Diff #204898)

I agree. It was like this before so I figured I would just follow how it was. Should I remove my part or the comment entirely?

rupprecht added inline comments.Jun 17 2019, 10:25 AM
llvm/tools/llvm-objcopy/CopyConfig.cpp
715 ↗(On Diff #204898)
  • The method is already commented in the header file; the .cpp files don't need comments. (Same for parseObjcopyOptions). They will inevitably go out of sync.
  • IMO it's fine to just not update the comment in this patch, and in a separate commit cleanup the header comments.
llvm/tools/llvm-objcopy/llvm-objcopy.cpp
85 ↗(On Diff #204898)

This doesn't need to have a return value, it should be void.

abrachet marked an inline comment as done.Jun 17 2019, 11:24 AM
abrachet added inline comments.
llvm/tools/llvm-objcopy/llvm-objcopy.cpp
85 ↗(On Diff #204898)

This was per @jhenderson’s suggestion, it lets the callback decide wether to continue parsing or not. Of course this one doesn’t do that right now, but allows for it in the future. I don’t have strong feelings either way.

rupprecht accepted this revision.Jun 17 2019, 11:41 AM
rupprecht marked an inline comment as done.
rupprecht added inline comments.
llvm/tools/llvm-objcopy/llvm-objcopy.cpp
85 ↗(On Diff #204898)

No, rereading that thread, I agree with how it's written. Makes sense to return Error so a different caller could treat warnings as fatal.

abrachet updated this revision to Diff 205222.Jun 17 2019, 5:31 PM

Removed comment in source file, keeping only the comment in the header.

abrachet marked 3 inline comments as done.Jun 17 2019, 5:32 PM
This revision was automatically updated to reflect the committed changes.