[PATCH] D67137: [llvm-objcopy] Support --only-keep-debug

Wed Oct 30 02:59:08 PDT 2019

jhenderson added a comment.

Only two minor nits from me currently, plus these comments which I made out-of-line:

> please make sure the docs and help text are updated

> Also, for my own benefit, would you mind resummarising the expected behaviour of --only-keep-debug. It might be useful to add this as a comment in the code somewhere too.

================
Comment at: llvm/test/tools/llvm-objcopy/ELF/only-keep-debug.test:68
+    Address:      0x1480          # Ensure Address=0x1000+Offset
+    AddressAlign: 128
+    # An arbitrary non-zero Size tests that .tdata does not occupy space
----------------
You use hex for AddressAlign above and also for Addresses. I think it would make sense to make it hex here and throughout the rest of the file too.

================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:1961
+    // The first section in a PT_LOAD has to have congruent offset and address
+    // modulo the alignment, which usually equals to the maximum page size.
+    if (FirstSec && FirstSec == &Sec)
----------------
Nit: "equals to" -> "equals" or "is equal to"

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67137