Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

jhenderson (James Henderson)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 18 2017, 2:49 AM (349 w, 5 d)

Recent Activity

Tue, Sep 26

jhenderson accepted D149759: [symbolizer] Support symbol lookup.

LGTM, thanks, but please give @ikudrin/@MaskRay/... a few days to make any other comments.

Tue, Sep 26, 12:00 AM · Restricted Project, Restricted Project

Mon, Sep 25

jhenderson accepted D139864: [llvm-cxxfilt] Do not consider the prefix dot as part of the demangled symbol name..

One remaining comment, then looks good to me

Mon, Sep 25, 11:37 PM · Restricted Project, Restricted Project
jhenderson added a comment to D139864: [llvm-cxxfilt] Do not consider the prefix dot as part of the demangled symbol name..

Could you not just modify nonMicrosoftDemangle to handle the dot prefix? That would mean this works for all tools, including llvm-cxxfilt, rather than needing this and the separate D159539 patch.

as I mention before, If I modify in `nonMicrosoftDemangle' as

bool llvm::nonMicrosoftDemangle(const char *MangledName, std::string &Result) {
  char *Demangled = nullptr;

  // Not consider the prefix dot as part of the demangled symbol name.
  if (MangledName[0] == '.') {
    ++MangledName;
    Result = ".";
  }

  if (isItaniumEncoding(MangledName))
    Demangled = itaniumDemangle(MangledName, nullptr, nullptr, nullptr);
  else if (isRustEncoding(MangledName))
    Demangled = rustDemangle(MangledName);
  else if (isDLangEncoding(MangledName))
    Demangled = dlangDemangle(MangledName);

  if (!Demangled)
    return false;

  Result += Demangled;
  std::free(Demangled);
  return true;

The llvm-nm or llvm-cxxfilt for machO will demangle "_._Z3f.0v" as ".f.0()"
but
[zhijian@krypton src]$ /opt/at15.0/bin/c++filt _._Z3f.0v
_._Z3f.0v

Mon, Sep 25, 1:19 AM · Restricted Project, Restricted Project

Fri, Sep 22

jhenderson added a comment to D139864: [llvm-cxxfilt] Do not consider the prefix dot as part of the demangled symbol name..

Could you not just modify nonMicrosoftDemangle to handle the dot prefix? That would mean this works for all tools, including llvm-cxxfilt, rather than needing this and the separate D159539 patch. I'm assuming that a dot prefix in front of a Microsoft mangled name doesn't make sense...

Fri, Sep 22, 1:24 AM · Restricted Project, Restricted Project
jhenderson added a comment to D159539: [NFC] refactor demangle of llvm-nm .

This patch should have been created using a GitHub Pull Request. See the big red notice at the top of the page...

Fri, Sep 22, 1:23 AM · Restricted Project, Restricted Project

Tue, Sep 19

jhenderson updated the diff for D151187: [doc] Add casting style preference to coding standards.

Update second bullet to match proposal.

Tue, Sep 19, 3:59 AM · Restricted Project, Restricted Project
jhenderson added a comment to D151187: [doc] Add casting style preference to coding standards.

When casting, use static_cast, reinterpret_cast, and const_cast rather than C-style casts. There are two exceptions to this:

  • When casting to void to suppress warnings about unused variables (as an alternative to `[[maybe_unused]]`). Prefer C-style casts in this instance.
  • When casting between integral types (including enums that are not strongly-typed), functional-style casts are permitted as an alternative to static_cast.

Reads good to me as well.

Updated patch based on my latest proposal. Feedback appreciated!

The changes look different from the proposal. Am I missing something?

Tue, Sep 19, 3:58 AM · Restricted Project, Restricted Project
jhenderson requested review of D151187: [doc] Add casting style preference to coding standards.
Tue, Sep 19, 2:37 AM · Restricted Project, Restricted Project
jhenderson updated the diff for D151187: [doc] Add casting style preference to coding standards.

Updated patch based on my latest proposal. Feedback appreciated!

Tue, Sep 19, 2:37 AM · Restricted Project, Restricted Project

Sun, Sep 17

jhenderson added a comment to D149757: Test data for symbol lookup. NFC.

Strong +1 to what both @ikudrin and @dblaikie have said: these files on their own don't do anything useful (not even add a test), so they shouldn't be added on their own in a separate commit/review. Please merge them into the review that uses them, or create the test that uses them, but showing the old behaviour instead (I think the former is my preference in this context).

Sun, Sep 17, 11:49 PM · Restricted Project, Restricted Project

Thu, Sep 14

jhenderson added a comment to D151187: [doc] Add casting style preference to coding standards.

I realised this dropped off my radar for a while, for various reasons, so I'm picking this back up in an effort to get a consensus and to land something. I'm not bothering uploading a diff at this point, but my most recent proposal was:

Thu, Sep 14, 1:38 AM · Restricted Project, Restricted Project

Wed, Sep 13

jhenderson accepted D150987: [llvm-nm] Add --line-numbers flag.

LGTM. Probably should wait for @MaskRay too.

Wed, Sep 13, 1:19 AM · Restricted Project, Restricted Project
jhenderson added a comment to D149759: [symbolizer] Support symbol lookup.

Sorry for the delay - Phabricator issues and being busy meant I only just got back to this.

Wed, Sep 13, 12:45 AM · Restricted Project, Restricted Project

Mon, Sep 11

jhenderson added inline comments to D150987: [llvm-nm] Add --line-numbers flag.
Mon, Sep 11, 1:33 AM · Restricted Project, Restricted Project

Fri, Sep 8

jhenderson added a reviewer for D139864: [llvm-cxxfilt] Do not consider the prefix dot as part of the demangled symbol name.: MaskRay.

My opinion hasn't changed that it's a mistake to put the dot demangling in llvm-cxxfilt specifically. That will mean you'll need to make the same change in llvm-nm, llvm-readelf, llvm-objdump and probably other tools too. Code duplication is bad. The next tool to be added might not realise that they can't just use the main demangler library as-is to do their name demangling.

Fri, Sep 8, 1:27 AM · Restricted Project, Restricted Project

Thu, Sep 7

jhenderson added a comment to D149759: [symbolizer] Support symbol lookup.

There look like there are a few of my previous comments that haven't been addressed yet?

Thu, Sep 7, 12:34 AM · Restricted Project, Restricted Project
jhenderson added inline comments to D150987: [llvm-nm] Add --line-numbers flag.
Thu, Sep 7, 12:18 AM · Restricted Project, Restricted Project

Wed, Sep 6

jhenderson accepted D144872: [AIX] Align the content of an xcoff object file which has auxiliary header in big archive. .

LGTM, with 2 comment nits and one small test issue that should be addressed before merging.

Wed, Sep 6, 12:31 AM · Restricted Project, Restricted Project

Tue, Sep 5

jhenderson added inline comments to D149759: [symbolizer] Support symbol lookup.
Tue, Sep 5, 1:03 AM · Restricted Project, Restricted Project

Mon, Sep 4

jhenderson added a comment to D150079: [BPF][DebugInfo] Show CO-RE relocations in llvm-objdump.

Only a couple of nits remaining from me.

Mon, Sep 4, 11:50 PM · Restricted Project, Restricted Project
jhenderson added inline comments to D144872: [AIX] Align the content of an xcoff object file which has auxiliary header in big archive. .
Mon, Sep 4, 12:43 AM · Restricted Project, Restricted Project
jhenderson added a comment to D149759: [symbolizer] Support symbol lookup.

@sepavloff, what's the situation with this patch? Are you planning on addressing review comments or similar, or waiting for an upstream patch to land etc etc?

Mon, Sep 4, 12:24 AM · Restricted Project, Restricted Project

Sep 1 2023

jhenderson accepted D159224: [llvm-objdump] Enable assembly highlighting in llvm-objdump.

LGTM. Please wait for @MaskRay too.

Sep 1 2023, 7:59 AM · Restricted Project, Restricted Project
jhenderson accepted D159162: [llvm] Add assembly syntax highlighting.

Looks reasonable to me, although it's been a while since I last looked at this sort of area.

Sep 1 2023, 12:55 AM · Restricted Project, Restricted Project
jhenderson added inline comments to D159224: [llvm-objdump] Enable assembly highlighting in llvm-objdump.
Sep 1 2023, 12:35 AM · Restricted Project, Restricted Project
jhenderson added inline comments to D144872: [AIX] Align the content of an xcoff object file which has auxiliary header in big archive. .
Sep 1 2023, 12:32 AM · Restricted Project, Restricted Project

Aug 31 2023

jhenderson added a comment to D159224: [llvm-objdump] Enable assembly highlighting in llvm-objdump.

Don't forget to add any new options to the llvm-objdump command guide.

Aug 31 2023, 12:27 AM · Restricted Project, Restricted Project
jhenderson added a comment to D149757: Test data for symbol lookup. NFC.

I wonder whether these files should go to cross-project-tests/

Aug 31 2023, 12:17 AM · Restricted Project, Restricted Project
jhenderson accepted D82172: Install llvm-readelf instead of llvm-readobj.
Aug 31 2023, 12:10 AM · Restricted Project, Restricted Project
jhenderson added a comment to D82172: Install llvm-readelf instead of llvm-readobj.

LGTM too, pending @MaskRay's requested fix.

Aug 31 2023, 12:10 AM · Restricted Project, Restricted Project

Aug 30 2023

jhenderson added a comment to D150079: [BPF][DebugInfo] Show CO-RE relocations in llvm-objdump.

I did a quick skim of comments and the like, but I don't really have the time to read up on BTF, so can't contirbute anything more meaningful really, sorry.

Aug 30 2023, 1:43 AM · Restricted Project, Restricted Project
jhenderson added inline comments to D144872: [AIX] Align the content of an xcoff object file which has auxiliary header in big archive. .
Aug 30 2023, 12:47 AM · Restricted Project, Restricted Project
jhenderson accepted D158004: llvm-nm ignore the Import symbol file for the --export-symbol option..

LGTM.

Aug 30 2023, 12:01 AM · Restricted Project, Restricted Project

Aug 29 2023

jhenderson accepted D157210: [symbolizer] Change reaction on invalid input.

LGTM, with one remaining nit.

Aug 29 2023, 7:05 AM · Restricted Project, Restricted Project
jhenderson added inline comments to D144872: [AIX] Align the content of an xcoff object file which has auxiliary header in big archive. .
Aug 29 2023, 2:34 AM · Restricted Project, Restricted Project
jhenderson added a comment to D157210: [symbolizer] Change reaction on invalid input.

Just some nits left from me.

Aug 29 2023, 2:13 AM · Restricted Project, Restricted Project
jhenderson added a comment to D158004: llvm-nm ignore the Import symbol file for the --export-symbol option..

Okay, thanks for the comments. I was trying to see if there was a sensible middle ground that would satisfy both my concerns and those of @MaskRay, but I don't think there really is at this point. Please could you revert to the previous llvm-nm implementation and drop the AIXImportFile stuff (make sure to address the test comments too). Sorry for the churn.

Aug 29 2023, 1:38 AM · Restricted Project, Restricted Project

Aug 25 2023

jhenderson added a comment to D158004: llvm-nm ignore the Import symbol file for the --export-symbol option..

Changing llvm::identify_magic to recognize all files starting with #! as aix_linker_import_file is not correct. Many can well be used as shebang for shell or other scripts.

case '#':
  if (Magic[1] == '!')
    return file_magic::aix_linker_import_file;
  break;

How would you suggest that @DiggerLin proceeds then? The impact of misidentifying a file with a shebang as an AIX import file is that tools will no longer report "invalid object file" or something to that effect for such files, but will otherwise do nothing with them, so it seems relatively harmless to me.

Sorry, I have analyzed the AIX use cases... I just noticed this patch and am concerned that identify_magic mischaracterizing files with shebang as AIX-specific would likely cause beyond-negligible negative impacts...
identify_magic in LLVMBinaryFormat is somewhat commonly used outside of llvm-project.

Aug 25 2023, 1:31 AM · Restricted Project, Restricted Project
jhenderson added a comment to D158004: llvm-nm ignore the Import symbol file for the --export-symbol option..

Changing llvm::identify_magic to recognize all files starting with #! as aix_linker_import_file is not correct. Many can well be used as shebang for shell or other scripts.

case '#':
  if (Magic[1] == '!')
    return file_magic::aix_linker_import_file;
  break;
Aug 25 2023, 12:57 AM · Restricted Project, Restricted Project

Aug 24 2023

jhenderson added a comment to D158560: access time is not reliably preserved on darwin.

Functionally, the change does what it says it does, and I have no issues with that. However, the discussion in the ticket seems to suggest this isn't a general issue in Darwin, but rather something about your system setup. That needs resolving before this lands.

Aug 24 2023, 11:49 PM · Restricted Project, Restricted Project
jhenderson added inline comments to D158004: llvm-nm ignore the Import symbol file for the --export-symbol option..
Aug 24 2023, 11:46 PM · Restricted Project, Restricted Project
jhenderson added a comment to D158799: [llvm-nm][WebAssembly] Report the size of data symbols.

Makes sense to me, but perhaps one additional test case needed. I'll leave @dschuff to give final approval though.

Aug 24 2023, 11:32 PM · Restricted Project, Restricted Project

Aug 23 2023

jhenderson added inline comments to D157210: [symbolizer] Change reaction on invalid input.
Aug 23 2023, 11:43 PM · Restricted Project, Restricted Project
jhenderson requested changes to D144872: [AIX] Align the content of an xcoff object file which has auxiliary header in big archive. .

I've added a few more nits. I've also gone through and highlighted a number of cases that I'm pretty confident don't have existing test coverage. I shouldn't have had to go through these bits myself and highlight all of this - you should have done this yourself, prior to the patch even going up for review. Requesting changes to clear the "Accepted" state.

Aug 23 2023, 12:23 AM · Restricted Project, Restricted Project

Aug 22 2023

jhenderson accepted D157210: [symbolizer] Change reaction on invalid input.

I took a break away from this for a few days, and decided that I shouldn't block this patch based on the behaviour change. However, I do think the specific case that can now hang (i.e. the case that caused the test to fail) should be highlighted in the release note, so that users can have a hint as to why their script started hanging after they updated to the latest llvm-symbolizer. LGTM with that.

Aug 22 2023, 11:57 PM · Restricted Project, Restricted Project
jhenderson removed a reviewer for D155775: [HIP][Clang][Driver][RFC] Add driver support for C++ Parallel Algorithm Offload: jhenderson.
Aug 22 2023, 11:51 PM · Restricted Project, Restricted Project, Restricted Project
jhenderson added inline comments to D158560: access time is not reliably preserved on darwin.
Aug 22 2023, 11:49 PM · Restricted Project, Restricted Project
jhenderson added a comment to D158429: Define BBEntry::hasIndirectBranch. NFC.

Looks good (should it have had a unit test?). Not sure why this was landed without review though?

Aug 22 2023, 1:04 AM · Restricted Project, Restricted Project
jhenderson added a comment to D158004: llvm-nm ignore the Import symbol file for the --export-symbol option..

Sorry, I've been trying to focus on other things for the majority of the last few days, as LLVM reviewing has been taking up too much of my worktime recently.

Aug 22 2023, 1:02 AM · Restricted Project, Restricted Project

Aug 18 2023

jhenderson added reviewers for D158164: [FileCheck] Added --match-full-lines-leading and --match-full-lines-trailing option: jdenny, thopre.

--match-full-lines is require to check that nothing trailing is missing. Current approach is to add in 100 lines {{$}}

Aug 18 2023, 1:22 AM · Restricted Project, Restricted Project
jhenderson added a comment to D158004: llvm-nm ignore the Import symbol file for the --export-symbol option..

I'd just like to make sure I follow what is going on here:

Aug 18 2023, 1:15 AM · Restricted Project, Restricted Project

Aug 17 2023

jhenderson added a comment to D158164: [FileCheck] Added --match-full-lines-leading and --match-full-lines-trailing option.

I'm not really seeing from your example why you need --match-full-lines at all. Are you just looking for the {{^}} and {{$}} regex patterns?

Aug 17 2023, 6:54 AM · Restricted Project, Restricted Project
jhenderson added a comment to D158164: [FileCheck] Added --match-full-lines-leading and --match-full-lines-trailing option.
Aug 17 2023, 6:50 AM · Restricted Project, Restricted Project
jhenderson added a comment to D158164: [FileCheck] Added --match-full-lines-leading and --match-full-lines-trailing option.

What's the motivation for this change?

Aug 17 2023, 6:36 AM · Restricted Project, Restricted Project
jhenderson added a comment to D139750: Optionally print symbolizer markup backtraces..

Looks like clang-format is complaining. Best make sure you've reformatted everything you've added using it.

Aug 17 2023, 12:12 AM · Restricted Project, Restricted Project

Aug 15 2023

jhenderson added a comment to D144872: [AIX] Align the content of an xcoff object file which has auxiliary header in big archive. .

I'll be honest, I'm not convinced the test coverage looks to be anywhere near comprehensive enough. However, I also haven't attempted to review it versus the code changes to check how much of the new code is actually covered. I'd suggest you consider using a code coverage tool to see how much coverage there is of your new code.

In the test big-archive-xcoff-auxi-head-align.test , it test whether the XCOFF object data member which has auxiliary header align correctly based on the information of auxiliary header. it is enough here. In AIX OS since build bot use CMake 3.22, it use llvm-ar instead of AIX OS ar. it will test the functionality too. (we have to add the -DCMAKE_AR=/usr/bin/ar when we compile, since llvm-ar do not align XCOFF object correctly for big archive in AIX os ). after the the patch commit, we will use the llvm-ar instead of the AIX ar

Aug 15 2023, 2:06 AM · Restricted Project, Restricted Project
jhenderson added inline comments to D144872: [AIX] Align the content of an xcoff object file which has auxiliary header in big archive. .
Aug 15 2023, 1:57 AM · Restricted Project, Restricted Project
jhenderson accepted D142660: [AIX] supporting -X options for llvm-ranlib in AIX OS.

I'm going to accept this change, although I still have significant concerns about how the whole parsing logic seems more complicated than it needs to be.

Aug 15 2023, 12:24 AM · Restricted Project, Restricted Project, Restricted Project
jhenderson added a comment to D157210: [symbolizer] Change reaction on invalid input.

My opinion is that if someone is in interactive mode and they get an error, it may make sense to exit llvm-symbolizer, rather than continue. How does this differ to the previous (prior to your patch) behaviour for other error cases (i.e. cases other than "missing an input file/address pair")?

This is behavior of llvm-symbolizer before this patch. Whenever it detects an invalid input it echoes the input and continue waiting on stdin. This behavior makes sense, - if the program got wrong command from interactive input, it can continue working because the next commands may do useful job.

Anyway, it is not llvm-symbolizer that caused the hang, it is blocking read in the test script, which waits input from the stream that is empty in the case of new llvm-symbolizer.

Aug 15 2023, 12:14 AM · Restricted Project, Restricted Project

Aug 14 2023

jhenderson added a comment to D157210: [symbolizer] Change reaction on invalid input.

Right, that's what I figured was the issue, but could you answer this part of my concern:

Aug 14 2023, 3:31 AM · Restricted Project, Restricted Project

Aug 13 2023

jhenderson requested changes to D157210: [symbolizer] Change reaction on invalid input.

As far as I can tell, the only "fix" you've made since the version that caused the hangs was to the test itself. That means that llvm-symbolizer could still hang under certain conditions surely, when it didn't used to...

Aug 13 2023, 11:53 PM · Restricted Project, Restricted Project
jhenderson accepted D149095: [llvm-objdump] Support CHPE code ranges in disassembler..

LGTM, thanks. I think it would be a good idea for @MaskRay to take another look given the fairly significant changes that happened since his approval.

Aug 13 2023, 11:46 PM · Restricted Project, Restricted Project

Aug 11 2023

jhenderson added a comment to D144872: [AIX] Align the content of an xcoff object file which has auxiliary header in big archive. .

I'll be honest, I'm not convinced the test coverage looks to be anywhere near comprehensive enough. However, I also haven't attempted to review it versus the code changes to check how much of the new code is actually covered. I'd suggest you consider using a code coverage tool to see how much coverage there is of your new code.

Aug 11 2023, 12:34 AM · Restricted Project, Restricted Project

Aug 10 2023

jhenderson accepted D156797: [llvm-readobj] [Object] [NFC] Introduce inline helpers for chpe_range_entry..

LGTM.

Aug 10 2023, 12:21 AM · Restricted Project, Restricted Project
jhenderson added a comment to D142660: [AIX] supporting -X options for llvm-ranlib in AIX OS.

clang-format is complaining in the pre-merge CI.

Aug 10 2023, 12:18 AM · Restricted Project, Restricted Project, Restricted Project
jhenderson requested changes to D149095: [llvm-objdump] Support CHPE code ranges in disassembler..

Windows pre-merge CI is still failing with a test related to this patch.

Aug 10 2023, 12:03 AM · Restricted Project, Restricted Project

Aug 9 2023

jhenderson accepted D157210: [symbolizer] Change reaction on invalid input.

Thanks for addressing my concerns. LGTM.

Aug 9 2023, 11:55 PM · Restricted Project, Restricted Project
jhenderson accepted D157203: [symbolizer][NFC] Reorganize parsing input binary file.

LGTM, with nits.

Aug 9 2023, 12:35 AM · Restricted Project, Restricted Project

Aug 8 2023

jhenderson requested changes to D157210: [symbolizer] Change reaction on invalid input.

Generally looks fine, but there are a number of small issues, and I'm not convinced by the JSON output behaviour change, hence requesting changes to clear the "accepted" state.

Aug 8 2023, 11:53 PM · Restricted Project, Restricted Project
jhenderson accepted D156978: [symbolizer][NFC] Move file argument parsing into separate function.

One nit from me, otherwise LGTM too.

Aug 8 2023, 11:40 PM · Restricted Project, Restricted Project
jhenderson added a comment to D154987: [lit] Implement PYTHON directive and config.prologue.

Thanks for the additional comments from everyone.

It looks like there's no reliable way to test what I was trying to test, and that test isn't *that* important anyway. I'll remove it since it was broken in the first place, so just rebase your patch. Sorry for the trouble.

No worries, and thanks for the fix. I finally got around to the rebase.

I think it would be worth outlining in this patch why simply calling an external python script (which could actually be embedded within the file) is insufficient for the motivating case.

+1

I just extended TestingGuide.rst. Let me know if it's not what you are looking for.

Aug 8 2023, 3:15 AM · Restricted Project, Restricted Project, Restricted Project

Aug 7 2023

jhenderson accepted D155943: [NFC] Update formatting of some symbolizer tests.

LGTM. Thanks for the improvement!

Aug 7 2023, 12:44 AM · Restricted Project, Restricted Project

Aug 1 2023

jhenderson added inline comments to D156797: [llvm-readobj] [Object] [NFC] Introduce inline helpers for chpe_range_entry..
Aug 1 2023, 11:27 AM · Restricted Project, Restricted Project
jhenderson accepted D156796: [ADT] [NFC] Introduce isLower and isUpper helpers..

LGTM. I'd add the unit test, but I'm not going to push strongly for it.

Aug 1 2023, 11:23 AM · Restricted Project, Restricted Project
jhenderson added a comment to D142660: [AIX] supporting -X options for llvm-ranlib in AIX OS.

As an alternative (but I think adds unnecessary complexity, due to needing an extra variable), you could have both tools read the environment variable into a string variable, then, if the -X option is present, overwrite that variable, and finally feed that string into the parsing code that converts into a BitMode value. If the string is invalid, the parsing code could report an error along the lines of "invalid OBJECT_MODE or -X option value".

if I do not think it is better than my current implement, If I implement as your suggestion, I need another variable to recoded the where the string come from(from OBJECT_MODE or -X option value). otherwise when the invalid value of string , how can I report it is an invalid OBJECT_MODE" or "invalid -X option value"

I'm not convinced you need to distinguish the two. If a user hasn't specified the -X option, then they'll know it's the OBJECT_MODE environment variable. Even if they have both, it should be obvious to a quick glance of the full error message what the wrong value is (because you'd include it in the message) and you could then it won't take long to find which of the two is wrong (even if they don't know that -X trumps OBJET_MODE). That being said, I still prefer the parse OBJECT_MODE first, report an error, then later read the -X option and check for an error there.

Aug 1 2023, 12:29 AM · Restricted Project, Restricted Project, Restricted Project

Jul 31 2023

jhenderson added a comment to D144872: [AIX] Align the content of an xcoff object file which has auxiliary header in big archive. .

Just as a heads-up, I'm off for the rest of the week. Hopefully I'll be able to get to any final points either before the end of my workday or later next week.

Jul 31 2023, 11:31 PM · Restricted Project, Restricted Project
jhenderson added a comment to D149095: [llvm-objdump] Support CHPE code ranges in disassembler..

Oh, the pre-merge check is failing on Windows too for a test from this patch.

Jul 31 2023, 11:10 PM · Restricted Project, Restricted Project
jhenderson added a comment to D149095: [llvm-objdump] Support CHPE code ranges in disassembler..

I'm off for the next few days, so I'm happy with this once the islower issue has been addressed (in a separate patch) and my comments addressed (in this patch).

Jul 31 2023, 11:07 PM · Restricted Project, Restricted Project
jhenderson added a comment to D155993: [llvm-debuginfod] Switch to xxh3_64bits.

I think the change makes sense, but I don't really know anything about debuginfod, so can't really be confident.

Jul 31 2023, 11:00 PM · Restricted Project, Restricted Project
jhenderson added inline comments to D144872: [AIX] Align the content of an xcoff object file which has auxiliary header in big archive. .
Jul 31 2023, 2:36 AM · Restricted Project, Restricted Project
jhenderson accepted D156622: [llvm-objdump] [NFC] Use a single vector to store all symbol mappings..

LGTM.

Jul 31 2023, 12:07 AM · Restricted Project, Restricted Project
jhenderson accepted D149095: [llvm-objdump] Support CHPE code ranges in disassembler..

No real objections from me to the latest version. I'll put a LGTM, but it should get reviewed by someone more familiar with CHPE and related things before you land it.

Jul 31 2023, 12:03 AM · Restricted Project, Restricted Project

Jul 30 2023

jhenderson added inline comments to D156603: [SymbolSize] Improve the performance of SymbolSize computation.
Jul 30 2023, 11:58 PM · Restricted Project, Restricted Project

Jul 28 2023

jhenderson added a comment to D149095: [llvm-objdump] Support CHPE code ranges in disassembler..

Only nits left from me, but someone with more familiarity with this stuff should review it.

Jul 28 2023, 12:55 AM · Restricted Project, Restricted Project
jhenderson accepted D149094: [llvm-objdump] [NFC] Use DisassemblerTarget for primary target in disassembleObject..

Thanks, LGTM.

Jul 28 2023, 12:50 AM · Restricted Project, Restricted Project
jhenderson added a comment to D142660: [AIX] supporting -X options for llvm-ranlib in AIX OS.

As an alternative (but I think adds unnecessary complexity, due to needing an extra variable), you could have both tools read the environment variable into a string variable, then, if the -X option is present, overwrite that variable, and finally feed that string into the parsing code that converts into a BitMode value. If the string is invalid, the parsing code could report an error along the lines of "invalid OBJECT_MODE or -X option value".

if I do not think it is better than my current implement, If I implement as your suggestion, I need another variable to recoded the where the string come from(from OBJECT_MODE or -X option value). otherwise when the invalid value of string , how can I report it is an invalid OBJECT_MODE" or "invalid -X option value"

Jul 28 2023, 12:34 AM · Restricted Project, Restricted Project, Restricted Project
jhenderson accepted D156454: [Object] [NFC] Use llvm::COFF::is64Bit in COFF import file implementation..

LGTM too.

Jul 28 2023, 12:08 AM · Restricted Project, Restricted Project

Jul 27 2023

jhenderson added inline comments to D156236: [RISCV] Make mapping symbols SF_FormatSpecific.
Jul 27 2023, 11:58 PM · Restricted Project, Restricted Project
jhenderson added a comment to D153376: Introducing llvm-cm: A Cost Model Tool.

My apologies for being slow in getting back to you - I had some time off and then have been busy catching up on all sorts of other reviews. By the way, feel free to ping the thread if it goes stale for a week.

Jul 27 2023, 1:39 AM · Restricted Project, Restricted Project
jhenderson added a comment to D142660: [AIX] supporting -X options for llvm-ranlib in AIX OS.

I don't see any reason to check the OBJECT_MODE environment variable if the -X flag is used. What would the error be: "You specified a valid -X flag, but by the way, OBJECT_MODE is set to an invalid value"?

Jul 27 2023, 12:18 AM · Restricted Project, Restricted Project, Restricted Project
jhenderson accepted D156190: [llvm-objdump] -d: don't display mapping symbols as labels.

LGTM. Perhaps worth a release note, and probably should wait for @jobnoorman to confirm.

Jul 27 2023, 12:08 AM · Restricted Project, Restricted Project
jhenderson accepted D155535: [WebAssembly][Objcopy] Write output section headers identically to inputs.

LGTM, with nits addressed.

Jul 27 2023, 12:04 AM · Restricted Project, Restricted Project

Jul 26 2023

jhenderson added inline comments to D156190: [llvm-objdump] -d: don't display mapping symbols as labels.
Jul 26 2023, 2:20 AM · Restricted Project, Restricted Project
jhenderson added inline comments to D142660: [AIX] supporting -X options for llvm-ranlib in AIX OS.
Jul 26 2023, 2:09 AM · Restricted Project, Restricted Project, Restricted Project
jhenderson added inline comments to D149094: [llvm-objdump] [NFC] Use DisassemblerTarget for primary target in disassembleObject..
Jul 26 2023, 12:42 AM · Restricted Project, Restricted Project
jhenderson added a comment to D149095: [llvm-objdump] Support CHPE code ranges in disassembler..

Unfortunately, my knowledge of COFF-related things is probably insufficient to review chunks of this code, so it would be best if you could get somebody else to review too, maybe @mstorsjo?

Jul 26 2023, 12:25 AM · Restricted Project, Restricted Project
jhenderson added a comment to D155535: [WebAssembly][Objcopy] Write output section headers identically to inputs.

Nearly there. Just a couple of small points now from me.

Jul 26 2023, 12:09 AM · Restricted Project, Restricted Project

Jul 25 2023

jhenderson accepted D156291: [llvm-objdump] Remove bool MachOOnlyFirst from printPrivateHeaders after D155045. NFC.

Thanks! LGTM.

Jul 25 2023, 11:58 PM · Restricted Project, Restricted Project
jhenderson added a comment to D155045: [llvm-objdump] Create ObjectFile specific dumpers.

This is definitely a big step in the right direction for llvm-objdump, in my opinion, and I look forward to further work in this area. My one concern with this patch is the Mach-O-specific parameter being passed into printPrivateHeaders, as that feels a bit ugly to me. I wonder if we could create the Mach-O dumper somehow with that parameter set in some manner? It would then no longer be a parameter of printPrivateHeaders that impacts all tools. Alternatively, there could be some other mechanism to give the dumper subclasses access to various configuration parameters.

Jul 25 2023, 8:40 AM · Restricted Project, Restricted Project
jhenderson added a comment to D154987: [lit] Implement PYTHON directive and config.prologue.

Just chiming in here to give my 2p. lit already has the ability to execute python scripts (call %python), but any such scripts are not directly tied into the lit, so running them etc won't produce output that fits with the lit diagnostics, as I understand it. As I understand it, this patch is essentially just an improvement of the "execute arbitrary python script" option. I'm in favour of it, but if it isn't already clear, I think it would be worth outlining in this patch why simply calling an external python script (which could actually be embedded within the file) is insufficient for the motivating case.

Jul 25 2023, 4:31 AM · Restricted Project, Restricted Project, Restricted Project