In D56571#1467358, @craig.topper wrote:

I think things don't work right unless you disable the integrated assembler. I'm not sure why though.

My opinion doesn't carry as much weight as others who are more familiar with the front-end code, but LGTM.

Move checks over to the main LL file, not Input file.

@pcc: Ping? :)

Another ping. Anyone? :-)

Friendly ping. :-)

In D56508#1397173, @rupprecht wrote:

In D56508#1397156, @void wrote:

That should be fixed by D57842, which I'm planning to submit soon -- I want to poke a few more test cases first.

This fails if the archives are nested. E.g.:

Ping? :-)

In D57267#1390484, @riccibruno wrote:

@void @rnk Perhaps you can comment on this: currently Expr::IgnoreImpCasts skips FullExprs, but Expr::IgnoreParenImpCasts only skips (via IgnoreParens) ConstantExprs. Is there any reason for this inconsistency ? I would like to add FullExpr to the nodes skipped by IgnoreParenImpCasts for consistency but I am worried about unexpected issues even though all tests pass.

LGTM, but may want to wait for other reviewers.

Move test to proper place and add "-verify" flag.

I encountered another ICE here: https://github.com/ClangBuiltLinux/linux/issues/320#issuecomment-455456506

There appears to be an ICE with this patch: https://github.com/ClangBuiltLinux/linux/issues/6#issuecomment-454675962

I like these changes. Could you add the documentation for the new instruction?

Apologies - the value seems to indeed overflow, but I'm still very confused how this was affected by this change.

Implement Rui's suggestion for how to output the ELF files.

Add check that the object file is still valid after using "obj-path".

In D56046#1341515, @MaskRay wrote:

The filenames of Task=0 and different. Is this preferable?

t.o t2.o were compiled with -module-summary

ld.lld --plugin-opt=obj-path=t3.o -shared t.o t2.o => creates t2.o0 t2.o1 t2.o2

gold --plugin=~/llvm/Release/lib/LLVMgold.so --plugin-opt=thinlto --plugin-opt=obj-path=t3.o -shared t.o t2.o => creates t2.o t2.o1 t2.o2

Don't append "0" to the object filename.

Add testcase.

In D56046#1341491, @ruiu wrote:

Replicating the gold behavior sounds fine, but does this patch work? This change seems to leave Buf uninitialized while Buf will be used later. Also, in gold LTO plugin, -obj-path overwrites -save-temps, but this change doesn't seem to replicate that behavior. Could you add tests?

Ping? :-)

In D55616#1335587, @vitalybuka wrote:

Looks like it's broken by this patch

clang: /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm/tools/clang/lib/AST/ExprConstant.cpp:11055: llvm::APSInt clang::Expr::EvaluateKnownConstInt(const clang::ASTContext &, SmallVectorImpl<clang::PartialDiagnosticAt> *) const: Assertion `Result && "Could not evaluate expression"' failed.

http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap-msan/builds/9281/steps/check-clang%20msan/logs/stdio

Fix how prefixes are used in the testcase.

Reduce duplication by using multiple check prefixes.

Ping? :-)

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.