np.

In D61694#1576962, @JamesNagurne wrote:

Maybe someone can enlighten me as to why the build bots aren't tripping up on this one, but our group is running into this when we pull this commit from the upstream:

In MCStreamer.h, the declaration of the pure virtual EmitSymbolAttribute is:

virtual bool EmitSymbolAttribute(MCSymbol *Symbol,
                                 MCSymbolAttr AttrKind,
                                 StringRef AttrVal = StringRef()) = 0;

In this revision, in MCXCOFFStreamer.h, the declaration is:

bool EmitSymbolAttribute(MCSymbol *Symbol, MCSymbolAttr Attribute) override;

This is ill-formed, because the 'override' declaration hides the pure virtual member function. Apparently the clang-fast build bots are ok with this, but I'm not sure how. A similar code segment (Compiler Explorer) fails on every clang compiler I've tried.

LGTM.

In D64197#1576519, @jsji wrote:

Reverted in https://reviews.llvm.org/rL365520.

In D64197#1576464, @sbc100 wrote:

I tried adding a dependency on TransformUtils to Analysis/LLVMBuild.txt but that generated:

CMake Error at CMakeLists.txt:711 (message):
  Unexpected failure executing llvm-build: llvm-build: fatal error: found
  cycle to u'Analysis' after following: u'AArch64CodeGen' (required library)
  -> u'Analysis' (required library) -> u'TransformUtils' (required library)
  -> Analysis

Does this mean that this change violates the dependency graph? Assuming you can repro, with -DBUILD_SHARED_LIBS=ON then perhaps we can revert this while a solution is found?

• Various comment tweaks and spelling fixes.
• Some indentation fixes and formatting changes.
• Removed unreachable return statement.
• Fixed error messages related to unsupported assembler pseudo ops.
• Added some comments to test clarifying why we are checking for .text csect, and that the binary and text outputs don't represent the same XCOFF content.

LGTM.

• Perform address arithmetic on const char* before casting to a uintptr_t.

• Lots of tweaks to comments and whitespace.
  • Added assertion to 'getPointerToSymbolTable`
  • Changed name of local variable in getSymbolSectionName to be more descriptive.
  • Use checkOffset and getObject'in parseStringTable.
  • Changed unreachables to report_fatal_error in 'printSectionHeaders`.
  • Added TODO where overflow checking eventually needs to be done.
  • Added comment about DWARF subsection types, & limited section flag printing to the section type flags.

LGTM.

Converted the way parsing the binary is done to not need an error_code out argument in the constructor. Creates a bare bones object file, then attempts to parse the expected structures out of the memory buffer. Trying to match the style ELF used.

Address further comments:

• Simplified the address checking in 'checkSectionAddress'
• Removed unneeded asserts. As Digger pointed out, an symbol related function that casts a DataRefImpl.p to a symbol table entry already asserts in toSymbolEntry.

Addressed first round of review comments.

• Fix getSectionFlags return value to be signed.
• Print the section flags as its symbolic value.

Addressed review comments:

• Fixed up several comments
• removed overridden functions that only called based class implementation.
• fixed up logic for creating an MCAsmInfo and Streamers so that Linux/ELF remains the default.
• Removed code for registering an XCOFFStreamer creation callback as no classes extend the XCOFF streamer.
• reviewed usage of assert/llvm_unreachable. Converted most to report_fatal_error
• Always use a timestamp of '0' for reproducible. Fatal error if incremental linking is enabled.

PowerPC64 changes LGTM.

Thank You @MaskRay.

@kamaub ^

In D63043#1538015, @MaskRay wrote:

@sfertile Sorry but I can't find commit 729111cf1824159bb4dd331cab8a829eab30313f or f49f58527a6d8147524d8d6f2eb1feb70f856292 in either llvm/llvm-project (git monorepo) or llvm-mirror/lld (git multirepo). Do you have the revision numbers, e.g. r362867 (or rL362867 rLLD362867; the latters give a clickable link on Phabricator)

In D63043#1537919, @ruiu wrote:

Let me revert this change.

Sean, feel free to revert a change if it broke buildbots like this. You don't have to go through a pre-commit code review for a revert change that fixes buildbots.

@MaskRay You broke the PowerpC64 build bot, and have been asked by both the buildbot maintainer (@anil9) and me to fix it. As of this morning the bot is still failing dues to the same problem (http://lab.llvm.org:8011/builders/ppc64le-lld-multistage-test/builds/4263/steps/build-stage2-unified-tree/logs/stdio) Can you please address that before committing PPC64 changes like this. I don't understand why you would consider committing more PowerPC64 changes when the bot is in this state, rather then fixing it first.

@MaskRay Are you planning on pulling this? The buildbot is still red and there have been numerous Power related changes going in with no buildbot coverage.

In D61583#1533390, @MaskRay wrote:

@sfertile I'm confused with the recent revert and revert of revert.. The problem this change caused should have been fixed by D62840. Did you find other problems? (The warn() can be deleted if you suspect there is somethin

LGTM.

LGTM.

ping.

LGTM.

ping

LGTM.

The "simplest" number 0x2000000 works no worse than other numbers so I'll just use it.

In D61647#1495325, @adalava wrote:

MaskRay, looks like many tests under lld/tests are using llvm-objdump, a grep for llvm-objdump on this folder returned >1000 entries.

In D61610#1494577, @peter.smith wrote:

If PPC is anything like Arm, I would be very surprised to see a lot of inter-section conditional branches,

Adding MaskRay as a reviewer for their familiarity with the llvm tools.

Thanks for posting this @adalava.

LGTM

Abandoning in favour of D60958.

Removed extra blank lines and added binaries into the diff.

Fixed return types to match the type of field being accessed.

LGTM.

Patch looks really good. I think you managed to explain the toc-indirection without getting to verbose which is something I've struggled with. I have a couple minor suggestions and I need to go over the test a bit more indepth but overall this looks good to me.

In D60878#1475619, @daltenty wrote:

The format of the binaries included in the patch don't seem correct, they create empty files. As a result the corresponding test are failing.

Hi MaskRay.

In D54720#1473968, @MaskRay wrote:

@sfertile Didn't catch you these days on IRC...

I've created D60958 for my idea.