Page MenuHomePhabricator

jhenderson (James Henderson)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 18 2017, 2:49 AM (208 w, 6 d)

Recent Activity

Today

jhenderson added inline comments to D94907: [llvm-nm][ELF] - Make -D display symbol versions..
Tue, Jan 19, 5:55 AM · Restricted Project
jhenderson added a comment to D94956: [yaml2obj/obj2yaml] - Improve dumping/creating of ELF versioning sections..

Looks to me like you need additional testing for when .dynsym or .dynstr are either not emitted at all, or are excluded from the section header table.

Tue, Jan 19, 5:53 AM · Restricted Project
jhenderson added inline comments to D94907: [llvm-nm][ELF] - Make -D display symbol versions..
Tue, Jan 19, 5:45 AM · Restricted Project
jhenderson added reviewers for D94481: [llvm-symbolizer][doc] Reorder --relativenames in options list: akhuang, phosek.

Ping?

Tue, Jan 19, 1:07 AM · Restricted Project
jhenderson added a comment to D94882: [MC] Upgrade DWARF version to 5 upon .file 0.

Maybe a silly suggestion (and one that would need more discussion on the mailing lists), but why not just change the default version to 5 more widely? How often do people deliberately deviate from the default DWARF version in practice? To me, the proposed change feels like a bit of a hack for what is likely fairly unusual usage, and is only temporary, since likely most toolchains will want to change to DWARF v5 at some point going forward.

ie: you're suggesting users needing to use DWARFv4 line tables would access that functionality with a flag passed to the assembler (as they do today if they want to opt in to DWARFv5)?

Right, yes, that's what I'm suggesting.

I think part of the problem is that people aren't used to (ie: build systems and other infrastructure isn't designed for) having to pass a dwarf version to their assembler - the line table format hasn't changed since DWARFv2. So if you have run -gN -S, you can pass the resulting file to the compiler/assembler without any flags. So even if we switch the default - then people who aren't ready to switch to DWARFv4 are going to be in a possibly difficult situation of trying to teach their build system how to pass the appropriate flags to the assembler, etc.

I think maybe the way to think about it is that there wouldn't be a default DWARF version - it could always be determined by the presence/absence of a file 0.

I'm concerned about relying on what is essentially a minor detail of the specification, for a couple of reasons:

  1. What if the .file 0 directive isn't present for future DWARF versions (e.g. DWARFv6), for whatever reason? You might have some code that is generated expecting DWARFv6, yet the assembler will end up generating (or at least trying to generate) DWARFv4 output. It seems like relying on a detail like this is fragile and not particularly future-proof.

Perhaps add an assembly directive when that that time comes?

Tue, Jan 19, 12:55 AM · Restricted Project
jhenderson added a comment to D94912: [llvm-nm][ELF] - Use @@ prefix when printing default versions..

Seems reasonable in principle.

Tue, Jan 19, 12:50 AM · Restricted Project
jhenderson added a comment to D94907: [llvm-nm][ELF] - Make -D display symbol versions..

As our downstream toolchain doesn't use symbol versioning, I'm not really familiar enough with this to review the details (and don't have the time to read back up on it currently), so you'll need @MaskRay or someone else to give input too.

Tue, Jan 19, 12:48 AM · Restricted Project
jhenderson added a comment to D94882: [MC] Upgrade DWARF version to 5 upon .file 0.

Maybe a silly suggestion (and one that would need more discussion on the mailing lists), but why not just change the default version to 5 more widely? How often do people deliberately deviate from the default DWARF version in practice? To me, the proposed change feels like a bit of a hack for what is likely fairly unusual usage, and is only temporary, since likely most toolchains will want to change to DWARF v5 at some point going forward.

ie: you're suggesting users needing to use DWARFv4 line tables would access that functionality with a flag passed to the assembler (as they do today if they want to opt in to DWARFv5)?

Tue, Jan 19, 12:39 AM · Restricted Project

Yesterday

jhenderson accepted D94771: [Object, llvm-readelf] - Move the API for retrieving symbol versions to ELF.h.

LGTM.

Mon, Jan 18, 12:54 AM · Restricted Project
jhenderson added a comment to D94560: [ELF] report section sizes when output file too large.

Thanks again.

I chose DAG on purpose because I think it more accurately expresses what we're really asserting.

First, we since we removed the sorting, we're not guaranteeing that these will be in any particular order. Given that, I would rather not make the test depend on the order.

Actually we give a guarantee: the order of output sections must be stable. When using -NEXT we verify that we print section sizes in the same order as they present in the file.
Using -DAG relaxes this check. Also, the current version of the test case looks like we have a sorting, though we don't.

So, personally I also think that using -NEXT is would be better here. Also wonder what @jhenderson thinks.

Mon, Jan 18, 12:44 AM · Restricted Project
jhenderson added a comment to D94882: [MC] Upgrade DWARF version to 5 upon .file 0.

Maybe a silly suggestion (and one that would need more discussion on the mailing lists), but why not just change the default version to 5 more widely? How often do people deliberately deviate from the default DWARF version in practice? To me, the proposed change feels like a bit of a hack for what is likely fairly unusual usage, and is only temporary, since likely most toolchains will want to change to DWARF v5 at some point going forward.

Mon, Jan 18, 12:38 AM · Restricted Project
jhenderson added inline comments to D93362: [llvm-elfabi] Support ELF file that lacks .gnu.hash section.
Mon, Jan 18, 12:31 AM · Restricted Project
jhenderson accepted D89845: Add the ability to extract the unwind rows from DWARF Call Frame Information..

Looks fine to me, with the caveat that I don't really know much about the functionality, so others with more knowledge should approve too.

Mon, Jan 18, 12:20 AM · Restricted Project
jhenderson added a comment to D94771: [Object, llvm-readelf] - Move the API for retrieving symbol versions to ELF.h.

Mostly looks fine. It might be nice if we could avoid putting so much code in the ELF.h header file, but that would require more refactoring, so we probably want to leave that to another patch.

Mon, Jan 18, 12:16 AM · Restricted Project

Fri, Jan 15

jhenderson added inline comments to D93362: [llvm-elfabi] Support ELF file that lacks .gnu.hash section.
Fri, Jan 15, 1:34 AM · Restricted Project
jhenderson added a comment to D94560: [ELF] report section sizes when output file too large.

I wonder whether the issue could be better solved by adding something (probably output section sizes as @grimar suggests) to the verbose output?

Not sure I understand what precisely you are suggesting. Are you saying to only show this information when --verbose is passed?

I think this is what @jhenderson meant, yes.

Fri, Jan 15, 1:27 AM · Restricted Project

Thu, Jan 14

jhenderson accepted D94667: [llvm-nm] - Move MachO specific logic out from the dumpSymbolNamesFromObject(). NFC..

Okay, LGTM!

Thu, Jan 14, 4:28 AM · Restricted Project
jhenderson accepted D94669: [llvm-nm] - Simplify the code in dumpSymbolNamesFromObject. NFC..

LGTM.

Thu, Jan 14, 4:28 AM · Restricted Project
jhenderson added inline comments to D94667: [llvm-nm] - Move MachO specific logic out from the dumpSymbolNamesFromObject(). NFC..
Thu, Jan 14, 4:25 AM · Restricted Project
jhenderson accepted D94659: [yaml2obj/obj2yaml] - Refine handling of SHT_GNU_verdef sections..

LGTM, with nit.

Thu, Jan 14, 4:21 AM · Restricted Project
jhenderson accepted D94660: [llvm-readobj][test] - Remove excessive YAML fields from tests..

Looks good.

Thu, Jan 14, 2:51 AM · Restricted Project
jhenderson added inline comments to D94659: [yaml2obj/obj2yaml] - Refine handling of SHT_GNU_verdef sections..
Thu, Jan 14, 2:49 AM · Restricted Project
jhenderson added a comment to D93362: [llvm-elfabi] Support ELF file that lacks .gnu.hash section.

Is it important that the .dynsym is used directly when .hash/.gnu.hash is available? At the moment, your test doesn't really show whether it is used or whether you go straight to the dynamic tags for details, I believe - to do that, you'd need to have one given bogus values for the size, and show that this is not sued. Additionally, you have no testing for your error messages, nor do you have testing that shows that the .gnu.hash is used above the .hash section.

Thu, Jan 14, 12:47 AM · Restricted Project
jhenderson added a comment to D94560: [ELF] report section sizes when output file too large.

I wonder whether the issue could be better solved by adding something (probably output section sizes as @grimar suggests) to the verbose output? That would allow us to keep the error message more concise, whilst still providing a developer the ability to answer the question of "where does the data go?"

Thu, Jan 14, 12:28 AM · Restricted Project

Wed, Jan 13

jhenderson added a comment to D93678: [yaml2obj] - Support selecting the location of the section header table..

That's a good point about the address/offset comment. I'm happy with the other idea then. @MaskRay, do you have any thoughts?

Wed, Jan 13, 1:55 AM · Restricted Project
jhenderson added a comment to D93678: [yaml2obj] - Support selecting the location of the section header table..

Option 3: Have a separate "Layout" block within the YAML which would allow you to define the layout. It might look a bit like this:

Layout:
  - FileHeader
  - Section1
  - ProgramHeaders
  - Section2
  - SectionHeaders
  - Section3

I'm not sure how this option would work alongside Offset values for sections, or what should happen if e.g. the file header entry were to be omitted.

If we really want to add a way to select where program headers are placed and also to support placing both program headers and section headers at an arbitrary offsets,
then I see the following solution:

Currently, internally, we have a list of Chunks, which consists of sections and a special Fill chunk.
If we rename the "Sections" YAML key to "Layout" (or alike), then we might be able to introduce 2 more special optional chunks: ProgramHeaders and SectionHeaders.

Then we will be able to write YAMLs like this:

--- !ELF
FileHeader:
  Class: ELFCLASS64
  Data:  ELFDATA2LSB
  Type:  ET_DYN
Layout:
  - Name: .section1
    Type: SHT_PROGBITS
    Size: 0
  - Type: SectionHeaders
     Offset: 0x1000
  - Name: .section2
     Type: SHT_PROGBITS
     Offset: 0x1200
  - Type: ProgramHeaders

I.e. it might look kind of close to what you've suggested in "Option 3", but doesn't introduce a one more separate YAML block, but allows to reuse the "Sections" block.

Perhaps, we can just have the "Sections" name unchanged for now and think about changing it (or keeping) later, when the functionality will be committed.

Wed, Jan 13, 1:04 AM · Restricted Project
jhenderson accepted D93858: [obj2yaml,yaml2obj] - Refine how we set/dump the sh_entsize field..

LGTM.

Wed, Jan 13, 12:41 AM · Restricted Project

Tue, Jan 12

jhenderson accepted D92923: [llvm-readelf/obj] - Add support of multiple SHT_SYMTAB_SHNDX sections..

LGTM.

Tue, Jan 12, 6:02 AM · Restricted Project
jhenderson requested review of D94481: [llvm-symbolizer][doc] Reorder --relativenames in options list.
Tue, Jan 12, 2:33 AM · Restricted Project
jhenderson added inline comments to rG9ec72cfc61ad: [llvm-readef/obj] - Change the design structure of ELF dumper. NFCI..
Tue, Jan 12, 2:23 AM
jhenderson added inline comments to rG9ec72cfc61ad: [llvm-readef/obj] - Change the design structure of ELF dumper. NFCI..
Tue, Jan 12, 2:21 AM
jhenderson added inline comments to rG9ec72cfc61ad: [llvm-readef/obj] - Change the design structure of ELF dumper. NFCI..
Tue, Jan 12, 2:14 AM
jhenderson added inline comments to D92923: [llvm-readelf/obj] - Add support of multiple SHT_SYMTAB_SHNDX sections..
Tue, Jan 12, 1:46 AM · Restricted Project
jhenderson accepted D93900: [llvm-readef/obj] - Change the design structure of ELF dumper. NFCI..

LGTM.

Tue, Jan 12, 1:32 AM · Restricted Project
jhenderson accepted D93854: [obj2yaml,yaml2obj] - Fix issues with creating/dumping group sections..

LGTM.

Tue, Jan 12, 12:38 AM · Restricted Project
jhenderson accepted D93697: [obj2yaml] - Don't crash when an object has an empty symbol table..

LGTM.

Tue, Jan 12, 12:37 AM · Restricted Project
jhenderson added a comment to D93362: [llvm-elfabi] Support ELF file that lacks .gnu.hash section.

I rebased the code, it seems upset the Phabricator since it does not distinguish between rebase changes and actual changes.

Tue, Jan 12, 12:37 AM · Restricted Project

Mon, Jan 11

jhenderson added inline comments to D89071: [SystemZ/z/OS] Add GOFFObjectFile class and details of GOFF file format.
Mon, Jan 11, 12:06 AM · Restricted Project

Fri, Jan 8

jhenderson added a comment to D89071: [SystemZ/z/OS] Add GOFFObjectFile class and details of GOFF file format.

I've not reviewed the testing yet, but my immediate thought is that there needs to be a lot more, handling all the different code paths.

Fri, Jan 8, 1:02 AM · Restricted Project
jhenderson added a comment to D93362: [llvm-elfabi] Support ELF file that lacks .gnu.hash section.

The testing for this is going to be somewhat similar to various llvm-readobj examples, which use yaml2obj to generate an ELF object with the required properties. For example, llvm\test\tools\llvm-readobj\ELF\dyn-symbols-size-from-hash-table.test contains testing for the llvm-readobj handling of dynamic symbols when there are no section headers. There are actually further improvements that can be made beyond that. For example, yaml2obj has recently gained the ability to not emit the section header table, rather than needing to use llvm-strip to remove it. See llvm\test\tools\yaml2obj\ELF\section-headers.yaml for an example.

Fri, Jan 8, 12:52 AM · Restricted Project

Thu, Jan 7

jhenderson accepted D93346: [Test][FileCheck] Fix use of undef var.

LGTM.

Thu, Jan 7, 11:58 PM · Restricted Project
jhenderson added a comment to D93362: [llvm-elfabi] Support ELF file that lacks .gnu.hash section.

friendly ping.

Thu, Jan 7, 1:03 AM · Restricted Project
jhenderson accepted D93341: [FileCheck] Do not skip end of line in diagnostics.

Personally, I think the new behaviour you're adding makes more sense. The old behaviour could get particularly confusing, I imagine, when matching literal tabs/spaces (e.g. due to --strict-whitespace) etc if the marker moves on later than expected. LGTM, but I think it would be good to get one or two others to give their input.

Thu, Jan 7, 12:38 AM · Restricted Project
jhenderson added inline comments to D93346: [Test][FileCheck] Fix use of undef var.
Thu, Jan 7, 12:28 AM · Restricted Project
jhenderson added a comment to D94141: [WIP][LLD] Add output section printout.

Could you give a step-by-step example of how you might use the information you want to emit? That will help us ensure that any proposals satisfy the requirements (or possibly to suggest an alternative).

Thu, Jan 7, 12:01 AM · Restricted Project

Wed, Jan 6

jhenderson added inline comments to D92923: [llvm-readelf/obj] - Add support of multiple SHT_SYMTAB_SHNDX sections..
Wed, Jan 6, 1:26 AM · Restricted Project
jhenderson added a comment to D93881: [llvm-objcopy] preserve file ownership when overwritten.

If this brings us closer to GNU behaviour, that's fine, but I do find it weird that a build system would be doing anything involving sudo to run llvm-objcopy/llvm-strip.

Wed, Jan 6, 12:11 AM · Restricted Project
jhenderson added a comment to D89845: Add the ability to extract the unwind rows from DWARF Call Frame Information..

Phabricator is still saying there are unit tests failing. Are these up-to-date reports, or is Phabricator stale?

Wed, Jan 6, 12:03 AM · Restricted Project
jhenderson added a comment to D94141: [WIP][LLD] Add output section printout.

I agree with @MaskRay: does the map file solve the problem for you?

Wed, Jan 6, 12:01 AM · Restricted Project

Tue, Jan 5

jhenderson accepted D93754: [obj2yaml] - Fix the crash in getUniquedSectionName()..

LGTM.

Tue, Jan 5, 1:37 AM · Restricted Project
jhenderson accepted D93753: [obj2yaml][test] - Improve and fix section-group.yaml test..

LGTM!

Tue, Jan 5, 1:33 AM · Restricted Project
jhenderson added a comment to D93854: [obj2yaml,yaml2obj] - Fix issues with creating/dumping group sections..

Seems to me like you should have a dedicate yaml2obj test that shows the sh_entsize can be set for group sections?

Tue, Jan 5, 1:31 AM · Restricted Project
jhenderson accepted D93805: [llvm-readelf/obj] - Index phdrs and relocations from 0 when reporting warnings..

LGTM!

Tue, Jan 5, 1:29 AM · Restricted Project
jhenderson updated subscribers of D93900: [llvm-readef/obj] - Change the design structure of ELF dumper. NFCI..

The original split of the two styles came in D14128, by @khemant. I can't see any explanation in that review for taking the route of using a separate style class as opposed to simply doing what you're doing now. Possibly it was just easier to retrofit it in that way. The style/dumper split would make sense, if the styles were shared between the different dumper kinds, but they aren't, and I suspect the dumping functionality for other tools may be too divergent for them to be easily reusable.

Tue, Jan 5, 1:05 AM · Restricted Project
jhenderson added a comment to D93858: [obj2yaml,yaml2obj] - Refine how we set/dump the sh_entsize field..

Could you explain why this patch fixes the ARM_EXIDX sh_entsize specifically? I don't see anything immediately that would only fix that section (and not other sections).

Tue, Jan 5, 12:45 AM · Restricted Project

Mon, Jan 4

jhenderson added inline comments to D93697: [obj2yaml] - Don't crash when an object has an empty symbol table..
Mon, Jan 4, 1:22 AM · Restricted Project
jhenderson added a comment to D93678: [yaml2obj] - Support selecting the location of the section header table..

If there's a case where this is useful, I definitely support it. More generally, we may want to be able to do the same with program header table placement. I don't know if we have a use-case for that yet, but it's probably worth keeping it in mind as we design this. BeforeSecData is therefore a little bit ambiguous here. For example, between the ELF header and program header table is also "before" the section data. Maybe the key words should be AfterEhdr, AfterPhdrs, AfterSections (or possibly AtEOF for the last one). We could probably not have one of AfterEhdr or AfterPhdrs for now. Longer-term, I could imagine it might be useful to allow arbitrary offsets for the section header table, so we should consider this in our design too.

Mon, Jan 4, 1:17 AM · Restricted Project
jhenderson added a comment to D80838: [llvm-ar] Add more tests for errors in opening archives.

Sorry for the delay in responding. I and I suspect others who were involved on this have been off over the Christmas period.

Mon, Jan 4, 12:55 AM · Restricted Project

Dec 18 2020

jhenderson accepted D92902: [llvm-elfabi] Add flag to keep timestamp when output is the same.

LGTM. Please wait for @grimar.

Dec 18 2020, 12:28 AM · Restricted Project

Dec 17 2020

jhenderson added a comment to D93362: [llvm-elfabi] Support ELF file that lacks .gnu.hash section.

What others said - there are six cases we should care about:

  1. Section headers are stripped and the ELF has no hash/gnu_hash dynamic tag - this can be treated as an error, I think since the ELF gABI requires a hash table.
  2. Section headers are stripped, but there is a DT_HASH
  3. Section headers are stripped, but there is a DT_GNU_HASH
  4. Section headers present but there is no .hash or .gnu.hash section - this might be treated as an error, I think since the ELF gABI requires a hash table.
  5. Section headers present with .hash section
  6. Section headers present with .gnu.hash section
Dec 17 2020, 4:38 AM · Restricted Project
jhenderson added a comment to D92902: [llvm-elfabi] Add flag to keep timestamp when output is the same.

FYI, my last day working this year is tomorrow. I won't be certain to be able to review anything now until the New Year.

Dec 17 2020, 4:30 AM · Restricted Project
jhenderson added inline comments to D93331: [ELF] Reject local-exec TLS relocations for -shared.
Dec 17 2020, 3:49 AM · Restricted Project
jhenderson accepted D93209: [libObject, llvm-readobj] - Reimplement `ELFFile<ELFT>::getEntry`..

LGTM, with nit.

Dec 17 2020, 3:43 AM · Restricted Project
jhenderson added a comment to D89845: Add the ability to extract the unwind rows from DWARF Call Frame Information..

Mostly looks fine, although there's a unit test failure, so not formally LGTMing. As I think I mentioned earlier, I'm not really familiar with the functional aspects of this change, so don't rely on me alone anyway. Tomorrow will be my last day in the office for over two weeks, so feel free to land if you think others have reviewed sufficiently after the remaining test fix.

Dec 17 2020, 3:39 AM · Restricted Project

Dec 16 2020

jhenderson added inline comments to D89071: [SystemZ/z/OS] Add GOFFObjectFile class and details of GOFF file format.
Dec 16 2020, 1:38 AM · Restricted Project
jhenderson added a comment to D89845: Add the ability to extract the unwind rows from DWARF Call Frame Information..

Looks fine aside from the two outstanding comments re. error messages.

I commented that these are in dump functions that were already created before this patch. They are in the process of dumping the contents to the stream, so dumping the error messages to the stream seems most important. We don't want to stop dumping the information that people asked for due to running into a single error in a file as people will probably want see all of the errors in a file.

Dec 16 2020, 1:16 AM · Restricted Project
jhenderson accepted D93241: [AArch64InstPrinter] Change printADRPLabel to print the target address in hexadecimal form.

LGTM, but I wouldn't mind @psmith or someone else with more AArch64 knowledge confirming the calculation is correct.

Dec 16 2020, 1:03 AM · Restricted Project
jhenderson added a comment to D86222: Fix PR46880: Fail CHECK-NOT with undefined variable.

Given that I've gone cold on this change and need to go through it again properly, and the number of other reviews that are on my plate, in addition to higher priority internal work I need to do before the Christmas break (my last day is Friday), I doubt I'll get a chance to look at this until some time in the New Year. Sorry! If people feel it's important to land sooner than that, please don't let my lack of review block this (I can always do post-commit reviews if needed).

Dec 16 2020, 1:01 AM · Restricted Project
jhenderson accepted D93297: [lib/Object] - Make ELFObjectFile::getSymbol() return Expected<>..

LGTM, with @MaskRay's suggestion. We can work on bubbling up the Error/Expected in due course to get rid of the report_fatal_error calls.

Dec 16 2020, 12:56 AM · Restricted Project
jhenderson added a comment to D93044: [llvm-readobj/elf] - AArch64: Handle AARCH64_VARIANT_PCS.

One test suggestion. Otherwise, this looks fine.

Dec 16 2020, 12:52 AM · Restricted Project
jhenderson accepted D93010: [yaml2obj/obj2yaml] - Make Value/Size fields of Symbol optional..

Looks good, with one possible set of suggestions.

Dec 16 2020, 12:44 AM · Restricted Project
jhenderson accepted D92641: [llvm-readelf/obj] - Handle out-of-order PT_LOADs better..

LGTM.

Dec 16 2020, 12:37 AM · Restricted Project
jhenderson added a comment to D92923: [llvm-readelf/obj] - Add support of multiple SHT_SYMTAB_SHNDX sections..

Sorry, this somehow slipped off my radar, and I've run out of time to do any further significant reviewing today. I've skimmed it briefly, and the general approach looks fine. I'd like ideally to find a way to avoid needing to pass both a pointer and size around for the shndx table, even if it's just to wrap it in a new class, or change to Optional<ArrayRef<...>> or something. If you'd like me to take a more detailed look tomorrow, I can certainly do so.

Dec 16 2020, 12:34 AM · Restricted Project

Dec 15 2020

jhenderson added a comment to D92923: [llvm-readelf/obj] - Add support of multiple SHT_SYMTAB_SHNDX sections..

Sorry, this somehow slipped off my radar, and I've run out of time to do any further significant reviewing today. I've skimmed it briefly, and the general approach looks fine. I'd like ideally to find a way to avoid needing to pass both a pointer and size around for the shndx table, even if it's just to wrap it in a new class, or change to Optional<ArrayRef<...>> or something. If you'd like me to take a more detailed look tomorrow, I can certainly do so.

Dec 15 2020, 1:27 AM · Restricted Project
jhenderson added inline comments to D93044: [llvm-readobj/elf] - AArch64: Handle AARCH64_VARIANT_PCS.
Dec 15 2020, 1:17 AM · Restricted Project
jhenderson added a comment to D93259: [ELF] Error for out-of-range R_X86_64_[REX_]GOTPCRELX.

Looks good to me aside from @grimar's comment.

Dec 15 2020, 1:11 AM · Restricted Project
jhenderson added inline comments to D93209: [libObject, llvm-readobj] - Reimplement `ELFFile<ELFT>::getEntry`..
Dec 15 2020, 1:06 AM · Restricted Project
jhenderson added a comment to D89845: Add the ability to extract the unwind rows from DWARF Call Frame Information..

Looks fine aside from the two outstanding comments re. error messages.

Dec 15 2020, 1:03 AM · Restricted Project
jhenderson added inline comments to D92641: [llvm-readelf/obj] - Handle out-of-order PT_LOADs better..
Dec 15 2020, 12:59 AM · Restricted Project
jhenderson added inline comments to D93010: [yaml2obj/obj2yaml] - Make Value/Size fields of Symbol optional..
Dec 15 2020, 12:57 AM · Restricted Project
jhenderson added a comment to D93241: [AArch64InstPrinter] Change printADRPLabel to print the target address in hexadecimal form.

Looks like there are plenty of failing tests still?

Dec 15 2020, 12:52 AM · Restricted Project
jhenderson added a comment to D93209: [libObject, llvm-readobj] - Reimplement `ELFFile<ELFT>::getEntry`..

Does this change impact reading a dynamic symbol table for an object with no section header table? In such a case, the size is derived from the hash table, so I imagine it'll work the same, but ideally it should be tested.

Dec 15 2020, 12:26 AM · Restricted Project
jhenderson added inline comments to D89071: [SystemZ/z/OS] Add GOFFObjectFile class and details of GOFF file format.
Dec 15 2020, 12:10 AM · Restricted Project

Dec 14 2020

jhenderson added a comment to D93217: [llvm-readelf] - Don't print OS/Processor specific prefix for known ELF file types..

Obviously, this looks good to me, since this was my original suggestion, but it's probably best for @MaskRay or others to agree.

Dec 14 2020, 7:21 AM · Restricted Project
jhenderson added a comment to D92052: [MC][ELF] Accept abbreviated form with sh_flags and sh_entsize.

@jhenderson What's your opinion on backporting this?

Dec 14 2020, 7:19 AM · Restricted Project
jhenderson added a comment to D93096: [llvm-readelf] - Improve ELF type field dumping..

I guess it might be OK. It is probably a bit inconistent with GNU which does:

static char *
get_file_type (unsigned e_type)
{
  static char buff[32];

  switch (e_type)
    {
    case ET_NONE: return _("NONE (None)");
    case ET_REL:  return _("REL (Relocatable file)");
    case ET_EXEC: return _("EXEC (Executable file)");
    case ET_DYN:  return _("DYN (Shared object file)");
    case ET_CORE: return _("CORE (Core file)");

    default:
      if ((e_type >= ET_LOPROC) && (e_type <= ET_HIPROC))
	snprintf (buff, sizeof (buff), _("Processor Specific: (%x)"), e_type);
      else if ((e_type >= ET_LOOS) && (e_type <= ET_HIOS))
	snprintf (buff, sizeof (buff), _("OS Specific: (%x)"), e_type);
      else
	snprintf (buff, sizeof (buff), _("<unknown>: %x"), e_type);
      return buff;
    }
}
Dec 14 2020, 7:16 AM · Restricted Project
jhenderson added a comment to D93096: [llvm-readelf] - Improve ELF type field dumping..

We've just run into an unexpected side-effect of this change, when this got merged into our downstream branch. We have a number of new ET_* values in the OS-specific range. In our downstream branch, we've added new entries to ElfObjectFileType so that these values are printed nicely as Type: OurCustomType. However, with this change, this has changed to Type: OS Specific: (OurCustomType) which isn't ideal. We can probably live with it, but I wonder if a small refactor might be better to allow us to have a clean downstream patch without really harming upstream code. My initial suggestion would be to change to something like this:

Dec 14 2020, 5:56 AM · Restricted Project
jhenderson added inline comments to D93044: [llvm-readobj/elf] - AArch64: Handle AARCH64_VARIANT_PCS.
Dec 14 2020, 12:47 AM · Restricted Project
jhenderson added inline comments to D89845: Add the ability to extract the unwind rows from DWARF Call Frame Information..
Dec 14 2020, 12:41 AM · Restricted Project
jhenderson added a comment to D92052: [MC][ELF] Accept abbreviated form with sh_flags and sh_entsize.

As it is a regression and affects real-world code (https://bugs.llvm.org/show_bug.cgi?id=48201),
I like to backport it to LLVM 11. – Any reasons not to do so?

@tstellar Can this still go into LLVM 11.0.1? It is a very small fix for a regression and I see that https://bugs.llvm.org/show_bug.cgi?id=release-11.0.1
still lists 18 open bugs.

Dec 14 2020, 12:39 AM · Restricted Project
jhenderson added a comment to D92902: [llvm-elfabi] Add flag to keep timestamp when output is the same.

Could you clarify the actual intended use-case of this option? I'm just trying to understand the situation before suggesting more things, as I realise some suggestions aren't useful for specific use-cases.

Dec 14 2020, 12:37 AM · Restricted Project
jhenderson accepted D93096: [llvm-readelf] - Improve ELF type field dumping..

Looks good to me.

Dec 14 2020, 12:13 AM · Restricted Project

Dec 11 2020

jhenderson added a comment to D91761: [FileCheck] Add check modifier capability.

No more comments from me at this time, but as before, I'd prefer others to confirm they're happy with the concept.

Dec 11 2020, 6:48 AM · Restricted Project
jhenderson added inline comments to D91761: [FileCheck] Add check modifier capability.
Dec 11 2020, 6:33 AM · Restricted Project
jhenderson accepted D93033: [llvm-readobj] - For SHT_REL relocations, don't display an addend..

Seems reasonable to me. I guess in some of the existing tests, they should be checking the addend in the relocated data, but if they don't already, we aren't making the situation worse. LGTM, with nits.

Dec 11 2020, 6:31 AM · Restricted Project
jhenderson added a reviewer for D93044: [llvm-readobj/elf] - AArch64: Handle AARCH64_VARIANT_PCS: grimar.
Dec 11 2020, 6:23 AM · Restricted Project
jhenderson accepted D92052: [MC][ELF] Accept abbreviated form with sh_flags and sh_entsize.

@jhenderson – did I answer your questions - or did I misread them? Are the other issues?

Dec 11 2020, 6:15 AM · Restricted Project
jhenderson added inline comments to D89845: Add the ability to extract the unwind rows from DWARF Call Frame Information..
Dec 11 2020, 6:11 AM · Restricted Project
jhenderson added inline comments to D93010: [yaml2obj/obj2yaml] - Make Value/Size fields of Symbol optional..
Dec 11 2020, 6:04 AM · Restricted Project
jhenderson added inline comments to D92641: [llvm-readelf/obj] - Handle out-of-order PT_LOADs better..
Dec 11 2020, 5:59 AM · Restricted Project