In D85782#2236493, @MaskRay wrote:

Longer answer:

binutils 2.26 introduced a regression: R_X86_64_PC32 can no longer be used against a protected symbol https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=ca3fe95e469b9daec153caa2c90665f5daaec2b5

The original issue is "Copy relocation against protected symbol doesn't work".
I agree with Rich Felker (https://gcc.gnu.org/ml/gcc/2016-04/msg00168.html) and
Cary Coutant (https://sourceware.org/ml/binutils/2016-03/msg00407.html https://gcc.gnu.org/ml/gcc/2016-04/msg00158.html https://gcc.gnu.org/ml/gcc/2016-04/msg00169.html) that we should
keep using direct access against protected symbols and disallow copy relocations against protected symbols.

I appreciate that Cary Coutant and Rafael Ávila de Espíndola added diagnostics to gold and lld, respectively:

• gold (https://sourceware.org/bugzilla/show_bug.cgi?id=19823)
• lld (https://bugs.llvm.org/show_bug.cgi?id=31476)

@skan Perhaps you are in a good position to change the resolutions to the following issues:)

GCC 5 x86-64 introduced a regression (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65248) I suggested a fix in May, 2019: https://sourceware.org/pipermail/gcc/2019-May/229309.html
i386 was flagged as a reproduce (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55012)

__attribute__((visibility("protected"))) int a;
int foo() { return a; } // GCC>=5 uses R_X86_64_GOTPCREL/R_X86_64_REX_GOTPCRELX instead of R_X86_64_PC32

binutils 2.26 introduced a regression R_X86_64_PC32 can no longer be used against a protected symbol https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=ca3fe95e469b9daec153caa2c90665f5daaec2b5

I think this introduces an old issue when lowering the instruction.

Address review comments

In D83366#2138210, @MaskRay wrote:

This is: if else if else if -> switch. I think it is just a refactoring and I agree that it is the right thing to do, but I don't find complexity being simplified.

In D83366#2138183, @MaskRay wrote:

Note that the number of lines actually increases... so I am not sure this is "simplification"

Update comments

Related revision: D80822

Ping.

Refactor addX86AlignBranchArgs to be reused in CommonArgs.cpp

Move tests to x86-malign-branch.c

LGTM

LGTM (with one comment)

Rebase

LGTM

Address review comments

Address review comments

Address review comments (only test file changes)

In D77851#1989744, @reames wrote:

Has this been reverted? I see discussion of a fix being worked on, but has the change which triggered problems been reverted? I can't tell easily from the review history.

Add test for RISCV to prevent future regression

Feel free to add yourself as a reviewer if you'd like to.

@MaskRay Is this acceptable?

In D77851#1988272, @Razer6 wrote:

In D77851#1988191, @MaskRay wrote:

Which way do you think is better? Personally, I prefer the second one, since getAssembler().getBackend().relaxInstruction(Relaxed, STI, Relaxed) is called in a loop, the assumption that third argument of relaxInstruction is a fresh uninitialized MCInst is very strange.

Special casing RISC-V looks good to me.

A special case is probably also needed for the Hexagon and AMDGPU backend, which does similar stuff in their relaxInstruction implementation than the RISC-V backend.

Is their any reason why relaxInstruction takes three argurments? Since relaxed is given in two arguments which are aliased to the same variable, I would drop the third argument and let relaxInstruction to either modify the given instruction or assign it to a fresh one (RISC-V, Hexagon, AMDGPU case).

In D77851#1988272, @Razer6 wrote:

In D77851#1988191, @MaskRay wrote:

Which way do you think is better? Personally, I prefer the second one, since getAssembler().getBackend().relaxInstruction(Relaxed, STI, Relaxed) is called in a loop, the assumption that third argument of relaxInstruction is a fresh uninitialized MCInst is very strange.

Special casing RISC-V looks good to me.

A special case is probably also needed for the Hexagon and AMDGPU backend, which does similar stuff in their relaxInstruction implementation than the RISC-V backend.

Is their any reason why relaxInstruction takes three argurments? Since relaxed is given in two arguments which are aliased to the same variable, I would drop the third argument and let relaxInstruction to either modify the given instruction or assign it to a fresh one (RISC-V, Hexagon, AMDGPU case).

In D77851#1986486, @Razer6 wrote:

It seems that this change breaks the RISCV backend. RISCVAsmBackend::relaxInstruction assumes that the Relaxed parameter is a fresh uninitialized MCInst. With this change, invalid instructions with too many operands are generated.

In D77851#1988060, @skan wrote:

In D77851#1986486, @Razer6 wrote:

It seems that this change breaks the RISCV backend. RISCVAsmBackend::relaxInstruction assumes that the Relaxed parameter is a fresh uninitialized MCInst. With this change, invalid instructions with too many operands are generated. A similar problem probably happens for the Hexagon and for the AMDGPU backend.

/cc @asb also FYI that this breaks the RISCV backend.

Could you provide a new fail case that can be reproduced?

Address review comments

In D77851#1986486, @Razer6 wrote:

It seems that this change breaks the RISCV backend. RISCVAsmBackend::relaxInstruction assumes that the Relaxed parameter is a fresh uninitialized MCInst. With this change, invalid instructions with too many operands are generated. A similar problem probably happens for the Hexagon and for the AMDGPU backend.

/cc @asb also FYI that this breaks the RISCV backend.

Address review comments

Address part of review comments
Check the REX is not zero before emitting it

Address review comments

In D77971#1985752, @MaskRay wrote:

In D77971#1985717, @skan wrote:

In D77971#1981345, @MaskRay wrote:

If we consider instructions in non-SHF_EXECINSTR sections an undefined behavior.

I asked some experts about this, and got the answer that putting instructions in non-executable section is legal and their encoding values will be treated as data. Do you have other concern about this patch?

Can you add some information to the description? Ideally there should be an example that instructions in a non-executable section are useful.

Personally I like this resolution because that means we don't need to extend D78138 (bss like sections) to non-executable sections.

In D77971#1981345, @MaskRay wrote:

If we consider instructions in non-SHF_EXECINSTR sections an undefined behavior.

LGTM

In D78180#1983011, @pengfei wrote:

It's better to add NFC in the title.
The type of TSFlags is uint64_t instead of const uint64_t, is it possible this value been changed during function call?

Rebase

In D77971#1982735, @MaskRay wrote: