In D55616#1329301, @jyknight wrote:

This seems like a good start, but not complete.

"n" and "i" both should require that their argument is a constant expression. For "n", it actually must be an immediate constant integer, so setRequiresImmediate() should be used there. For "i", you may use an lvalue constant as well. The way we seem to indicate that, today, is with if (!Info.allowsRegister() && !Info.allowsMemory()). However the code in that block does not today *require* that the evaluation as a constant succeed...and it should.

It should also only require that the result is an integer when requiresImmediateConstant().

Additionally, the Sema code needs to have the same conditions as the CodeGen code for when a constant expression is required, but it doesn't. (before or after your change).

The n constriant *requires* a known integer. Therefore, enforce this in both
Sema and CodeGen by setting the "requires immediate" flag and evaluating to a
known integer instead of random "int"..

Addressed Eli's comment.

Don't look at the optimization level here.

In D54355#1328786, @jyknight wrote:

In D54355#1328557, @void wrote:

The issue is that "n" is expecting an immediate value, but the result of the ternary operator isn't calculated by the front-end, because it doesn't "know" that the evaluation of __builtin_constant_p shouldn't be delayed (it being compiled at -O0). I suspect that this issue will also happen with the "i" constraint.

Indeed. We _do_ actually already know that in clang too, however -- clang already knows that "n" requires an immediate, and does constant expression evaluation for it, e.g. to expand constexpr functions. Grep for "requiresImmediateConstant". I guess it's just not hooked up to the __b_c_p correctly, though.

(Note, as a workaround, you could use | instead of ||, or put the expression as the value of an enumeration constant).

As a side note, the number of ways to evaluate a constant expression are legion in clang. They should really be unified...

The issue is that "n" is expecting an immediate value, but the result of the ternary operator isn't calculated by the front-end, because it doesn't "know" that the evaluation of __builtin_constant_p shouldn't be delayed (it being compiled at -O0). I suspect that this issue will also happen with the "i" constraint.

In D54355#1326494, @craig.topper wrote:

I'm seeing a case where an inline assembly 'n' constraint is behaving differently in -O0 now. Previously we would evaluate the builtin_constant_p as part of the EvaluateAsInt call in CodeGenFunction::EmitAsmInput, but I think we're not doing that now. This causes us to fail later because the constraint wasn't satisfied since we were in -O0 and thus we didn't optimize the rest of the expression. The test case we have is a nasty mix of builtin_constant_p, builtin_choose_expr, and builtin_classify_type. I can see about sharing it if needed.

In D55311#1320727, @ruiu wrote:

Sorry, I'm really confused. Could you explain again for me why you need this patch to link Linux kernel?

In D55311#1320722, @grimar wrote:

In D55311#1320583, @void wrote:

In D55311#1320159, @ruiu wrote:

What is the motivation to do this? We don't try too hard to implement every detail of the linker script because it's just too complicated and there is usually an easy workaround that works without a linker script.

The motivation is to support linking Linux, which uses linker scripts heavily. I added a comment by IanT from another thread about why this is a good addition.

Now I remember. We saw this bug earlier and decided to report an issue to linux kernel and not change the linker.
Bug reported here: https://bugzilla.kernel.org/show_bug.cgi?id=194091.

In D55311#1320721, @ruiu wrote:

Looks like what you are trying to do is to generate a x86-64 executable from i386 object files. That's what we do not expect in lld. With this patch, in theory you can create AArch64 exectuables from x86-64 object files, but that doesn't make sense and likely to crash the linker because we assume that all object files are uniform in terms of target types.

I think this use case is too tricky to directly support in the linker. I'd probably use objcopy or something to transplant i386 code to x86-64 object file before passing it to the linker, so that we don't deal with the trickiness in the linker.

In D55311#1320699, @ruiu wrote:

Even with this patch, don't you still have to make a change to lld so that it accepts a set of input files of different targets?

In D55311#1320159, @ruiu wrote:

What is the motivation to do this? We don't try too hard to implement every detail of the linker script because it's just too complicated and there is usually an easy workaround that works without a linker script.

In D55311#1319943, @grimar wrote:

What is your use case? I wonder what is the reason to use OUTPUT_ARCH to override -m?

It was pointed out that the "-m" flag has slightly different semantics than what we would expect:

Note: I tried to add as many BFD arch entries as I could muster from binutils. It's obvious not complete, so any further entries you think should be here let me know.

In D54125#1314825, @tejohnson wrote:

In D54125#1314824, @tejohnson wrote:

In D54125#1314823, @tejohnson wrote:

In D54125#1314702, @tejohnson wrote:

In D54125#1314634, @tejohnson wrote:

In D54125#1313868, @void wrote:

To replicate the failure, do this:

$ llvm-ar rcsTD test-archive.o softirq.o irqdesc.o
$ ld.gold -plugin LLVMgold.so -plugin-opt=thinlto -plugin-opt=-code-model=kernel -plugin-opt=jobs=6 -plugin-opt=-stack-alignment=8 -m elf_x86_64 -r -o test-object.o --whole-archive test-archive.o
$ nm test-object.o  | grep W
0000000000001370 W arch_dynirq_lower_bound
00000000000001b0 W arch_early_irq_init
00000000000001a0 W arch_probe_nr_irqs
0000000000000190 W early_irq_init

Note that early_irq_init is still weak. It should have resolved to a concrete function.

I tried the reproducer and via -plugin-opts=save-temps I can see that the linker is telling LTO that the copy of early_irq_init in irqdesc.o is non-prevailing and that the softirq.o copy is prevailing. Therefore, with this patch the copy in irqdesc.o which is non-weak is dropped, and the weak copy in softirq.o is kept. So LTO seems to be doing what it should be based on what gold tells it.

The question is why is gold picking the weak symbol as the prevailing copy and not the strong one? Note that if the order of the object files is reversed in the llvm-ar invocation, the irqdesc.o copy of that symbol is the prevailing one as per the linker, and we keep the strong symbol instead.

Interestingly, lld behaves differently. Even with the softirq.o being put first in the archive as you have in your repro, it says that the version of early_irq_init in irqdesc is the prevailing copy, and the strong symbol is kept. Bug in gold?

I confirmed that the the llvm gold-plugin is telling gold that softirq.o:early_irq_init is a hidden weak def and that irqdesc.o:early_irq_init is a hidden strong def, and that gold is subsequently coming back and providing the following resolutions to the plugin for LTO:
softirq.o: early_irq_init: PREVAILING_DEF_REG
irqdesc.o: early_irq_init: PREEMPTED_IR

Interestingly, if I compile the .o files down to native objects, then go through the same llvm-ar and gold link sequence with them, gold does what you want: it keeps the strong def. So this seems to be a bug specific to gold's plugin handling. I'm not sure how to proceed, as the patch fixes a bug and is apparently just exposing a gold linker plugin handling bug.

ah - looks like this patch exposed an issue you already discovered for this same symbol and regular LTO:

http://lists.llvm.org/pipermail/llvm-dev/2018-October/127051.html

Was this ever reported to binutils/gold?

Actually - as Peter suggested there, this is in fact fixed by a more recent version of gold. The gold I use with llvm regression tests (GNU gold (GNU Binutils 2.30.51.20180214) 1.16) fixes this issue:

softirq.o: early_irq_init: PREEMPTED_IR
irqdesc.o: early_irq_init: PREVAILING_DEF_REG

$ nm test-object.o | grep W
0000000000001370 W arch_dynirq_lower_bound
00000000000001a0 W arch_early_irq_init
0000000000000190 W arch_probe_nr_irqs
$

My installed version of gold (1.15) has the bug. So please do use a more recent version of gold to fix this issue.

In D54125#1314824, @tejohnson wrote:

In D54125#1314823, @tejohnson wrote:

In D54125#1314702, @tejohnson wrote:

In D54125#1314634, @tejohnson wrote:

In D54125#1313868, @void wrote:

To replicate the failure, do this:

$ llvm-ar rcsTD test-archive.o softirq.o irqdesc.o
$ ld.gold -plugin LLVMgold.so -plugin-opt=thinlto -plugin-opt=-code-model=kernel -plugin-opt=jobs=6 -plugin-opt=-stack-alignment=8 -m elf_x86_64 -r -o test-object.o --whole-archive test-archive.o
$ nm test-object.o  | grep W
0000000000001370 W arch_dynirq_lower_bound
00000000000001b0 W arch_early_irq_init
00000000000001a0 W arch_probe_nr_irqs
0000000000000190 W early_irq_init

Note that early_irq_init is still weak. It should have resolved to a concrete function.

I tried the reproducer and via -plugin-opts=save-temps I can see that the linker is telling LTO that the copy of early_irq_init in irqdesc.o is non-prevailing and that the softirq.o copy is prevailing. Therefore, with this patch the copy in irqdesc.o which is non-weak is dropped, and the weak copy in softirq.o is kept. So LTO seems to be doing what it should be based on what gold tells it.

The question is why is gold picking the weak symbol as the prevailing copy and not the strong one? Note that if the order of the object files is reversed in the llvm-ar invocation, the irqdesc.o copy of that symbol is the prevailing one as per the linker, and we keep the strong symbol instead.

Interestingly, lld behaves differently. Even with the softirq.o being put first in the archive as you have in your repro, it says that the version of early_irq_init in irqdesc is the prevailing copy, and the strong symbol is kept. Bug in gold?

I confirmed that the the llvm gold-plugin is telling gold that softirq.o:early_irq_init is a hidden weak def and that irqdesc.o:early_irq_init is a hidden strong def, and that gold is subsequently coming back and providing the following resolutions to the plugin for LTO:
softirq.o: early_irq_init: PREVAILING_DEF_REG
irqdesc.o: early_irq_init: PREEMPTED_IR

Interestingly, if I compile the .o files down to native objects, then go through the same llvm-ar and gold link sequence with them, gold does what you want: it keeps the strong def. So this seems to be a bug specific to gold's plugin handling. I'm not sure how to proceed, as the patch fixes a bug and is apparently just exposing a gold linker plugin handling bug.

ah - looks like this patch exposed an issue you already discovered for this same symbol and regular LTO:

http://lists.llvm.org/pipermail/llvm-dev/2018-October/127051.html

Was this ever reported to binutils/gold?

To replicate the failure, do this:

In D54125#1313855, @pirama wrote:

In D54125#1313836, @void wrote:

Hi @piramam,

This change is breaking the Linux ThinLTO build. I'll work to get you a test case. Could you revert this patch until we can resolve the issue?

This patch fixed a longstanding break with ThinLTO + PGO + LLD. If the current issue is easy to fix, I'd prefer to fix forward rather than revert this patch. Can you share details on what fails - error message or crash?

But ultimately, this is @tejohnson's call to revert or fix forward.

This change is breaking the Linux ThinLTO build. I'll work to get you a test case. Could you revert this patch until we can resolve the issue?

Fixed in r347531.

Doh! I'll take a look at it.

Should be okay now. PTAL. :-)

Remove ODR violation.

No function pointers

Don't re-wrap a ConstExpr.

Ping? I really don't want this review to go on forever.

Ping? :-)

ImpCast.

Fix overflow flag.

I think this is ready now. PTAL.

Updated to address comments.

Remove some extraneous checks.

In D54356#1297543, @rsmith wrote:

In D54356#1297522, @void wrote:

Okay. I'll revert this then.

I don't think we necessarily need the change in the other patch that's based on this one, but I still think this refactoring is an improvement :)

This isn't necessary. We can assume it's in a constant context because it's checking for an ICE.

Okay. I'll revert this then.

This code is called in over 90 places, so it's hard to tell if they all are in a constant context. Though I suppose that what this code is supposed to check for would work the same in a constant context as without one. I can revert this if you want, but to be honest the original function was terrible--it's huge and hard to understand what's going on. As for adding new expressions, this isn't the only place where a StmtVisitor is used. One still needs to go through all of those visitors to see if they need to add their expression.

Ping?

Ping?

This has D54356 integrated into it. D54356 should be reviewed and submitted first, even though it's out of order.

Adding ConstantExpr visitor.

I'm using "arcanist" for the first time, and I don't want to mess things up. I'll address the comments here in the next patch.

I believe this patch should address most, if not all, of your concerns. PTAL.