In D87121#2261176, @MaskRay wrote:

In D87121#2261061, @shenhan wrote:

Hi Maskray, thanks for the fix. Another latent concern is that apply the symbol ordering file unconditionally might cause unintentional consequence to the final binary to the extent that the binary might be corrupted or less optimized, in this sense, this patch might still be warranted (orthogonal to your CL), what do you think?

I think it is important for the user to specify the appropriate linker flags. None of the other flags (even similar ones, e.g. --dynamic-list, -T, --version-script) should have such an exclusion mechanism (one concern is that changing -o can change the behavior, which is not great). There may be someway for the mysql build system to not apply the linker options at config stage, or you should just hook the final link. For your case --no-warn-symbol-ordering can essentially nullify the time increase caused by --symbol-ordering-file.

Hi Maskray, thanks for the fix. Another latent concern is that apply the symbol ordering file unconditionally might cause unintentional consequence to the final binary to the extent that the binary might be corrupted or less optimized, in this sense, this patch might still be warranted (orthogonal to your CL), what do you think?

Hi Fangrui, I've put up a reproduce case for mysql here. These are steps:

In D85408#2253055, @MaskRay wrote:

I am still reading the patch, but I have noticed one thing worth discussing. .bb_addr_map is a non-SHF_ALLOC section (meaning that it is not part of the memory image). An absolute relocation type (.quad .Lfunc_begin0) works but the value is a link-time address, not taking account of the load base (PIE/shared object)). (If .bb_addr_map has the SHF_ALLOC flag, linkers will report a text relocation) How do you intend to use .bb_addr_map at runtime?

Hi, would you to take another look? Thanks, -Han

Thanks Rui. Uploaded the patch: 1) removed PropellerConfig 2). Removed PropellerCommon.h from patch (and moved it into llvm realm).

Fixed all variable names to comply with lld naming convention.

Hi Markray, thanks for the review.

Hello, over the past 2 months since our first draft of Propeller, we've made huge changes/improvements and this describes what happens -http://lists.llvm.org/pipermail/llvm-dev/2020-January/138426.html

Added lld/ELF/Propeller/README.md. Moved some of file comments into README.md.

Hi, we've refactored propeller and cleaned up dependencies and interfaces. Here are the change summaries:

Updated header file description. Also changed local variable names to lower case camel.

In D68062#1683618, @ruiu wrote:

Let me start from very high-level review comments before getting into the details of the actual code:

• First and foremost, can you add a file comments to each file to explain what you are doing in the file? It doesn't have to be detailed, but giving an overview at the beginning of a file is generally very useful.

Addressed maskray's comments.

@ruiu whatś your opinion on this?

Re-write the getAlignment function, keep the original logic but with the 3 additional tests:

Mind take another look?

@pcc thanks for the review. Updated the patch -

1. if getAlignment sees a ABS symbols, returns 0
2. initialize Ret to UINT64_MAX
3. in "addCopyRelSymbol", errors out if alignment is 0.
4. unit test case that tests for proper handling of ABS symbols.

Thanks @ruiu Updated the patch, also attached a test case.

@pcc do you agree with initializing Ret to uint64-1?

@pcc

Thanks Rui. I just updated the patch a little bit, that - st_value is only valid for alignment when symbol shndx is set to SHN_COMMON.

In D47396#1113043, @ruiu wrote:

+Han Shen <shenhan@google.com>

Han, did you try that idea before?

Thanks. (Comments revised and DefaoutOutSec -> Default)

Hi George, mind take a look? (This is a follow up of D45788.)

Thanks for the review. Fixed as suggested in D45788.

Merge D46764 (Fix test cases that check addresses that are not always invariable) into this.

Sorry for breaking the builders. Should double check again after my fix for the broken 4 cases.

Some clarification why my test result is different from George's.