Please verify that the following program spins inside function foo.

LGTM. It seems we are the only people who are interested in this patch. Please go ahead and submit. Thank you for doing this!

• Fix test.

• Updated as per pcc's comments.

LGTM

LGTM

You want to rebase first as I changed the types of the variables in this file as I used make<>.

Did you try to run this code on a simulator?

Nice! I like this.

LGTM, but you want to wait until next week for Vassil.

LGTM

LGTM

Thank you for verifying! LGTM.

Is this behavior the same as the MSVC linker? If it doesn't make sense to pass the /pdb option without /debug, we might want to emit an error (or warning) in the driver.

If you are adding code to handle dynamic arguments, then yes, we probably need to define what is the first incremental step towards the goal after discussing how to handle dynamic arguments. But this patch seems somewhat orthogonal to that, no? This doesn't do anything about dynamic arguments and makes sense without a follow-up patch. It adds only 12 lines of code to OptTable.cpp, so "completely revising" it shouldn't be a big deal at all. So I believe this is something we usually submit incrementally.

Why do you want to not submit this alone but wait for another patch? In-tree incremental development is a normal thing, and the feature this patch provides is already useful without any further changes, so I don't see a reason to wait.

I'm fine with these nits. (Am I supposed to sign off?)

As to builtin functions for AVR that GCC has but Clang doesn't, you might want to add that to Clang, but that is not directly related to LLD. So I'll defer that question to someone more familiar with Clang. If you need to use GCC to generate object files to test LLD, just use GCC.

I've submitted this change by mistake, but I got OK from Peter verbally.

That should theoretically work, but I can imagine that that will be more complicated than this. Chunks for dllimported symbols are created not when they are read from file but when writer needs them, so at the time when MarkLive runs, chunks for dllimported symbols don't exist.

On second thought, it isn't good to discuss new linker command line option in a code review thread, as most people do not read every review threads. Please start a thread in llvm-dev to propose a new option with an explanation why you want it.

I have a couple of questions.

I think I'm not convinced. Using multi-threading when you are optimizing it for single-thread is distracting and increases deviation when you measure its performance. Please keep it as your local patch if you need it.

LGTM

Did you try to run a program generated with this patch on an actual AVR (or a simulator)? Did it work?

I'd recommend renaming ResFile WindowsRes or WindowsResource everywhere, as I don't think Res or ResFile sound very familiar even for those who have an experience of Windows development, and for those who don't know about it, Res has no evidence about what that is.

Okay, it is not at least as concerning as I first thought. LGTM.

LGTM

LGTM, but I wonder if someone else wants to see this patch. This is code that I wrote, and Martell is moving it to LLVM, so it's great if we can get 3rd party's review.

LGTM. Let's land this first to fix the crash bug.

Fix all formatting errors before uploading any new patch, please.

LGTM with a comment describing threads are disabled for this test because it's too slow otherwise.

This looks better, but how does it work? grep -w -r DWARFAddress . shows nothing in my LLVM repository.

How did you create that object file? At least you want to leave a comment in the test about how you created it.

And at the same time I would really want to ask you to land this patch too, because it is usefull for comparsion purposes and does not interfere with idea to optimize single threaded solution, but raises whole
target perfomance on a new level and gives some point we can reference to.

This looks mostly fine, but I wouldn't submit this patch at the moment, as I think the code to generate .gdb_index is not optimize enough for single-thread yet. Our general strategy is to focus on single-thread performance and then use multi-threads as a final shot. I believe this strategy is working well, because (a) using multi-threads may hide real problems if used too early that are obvious when run with only one thread (which would result in making the linker a so-so performance), and (b) thinking about multi-threading is distracting when optimizing code.

I'm not in favor of this change. I don't want to introduce a new class in general unless it is worthwhile. It seems this patch introduced a new class hierarchy too casually.

Did you know why that test takes that much time with threading?

LGTM

This looks overall good, a few minor nits.

LGTM

LGTM

The new ELF linker is completely different from the old one, and the new one is by far simpler than before. It should be much much easier to add a new architecture to the linker. So, in many cases it doesn't make sense or irrelevant to try to find a mapping from the old linker infrastructure to the new one to figure out what to do. Most boilerplates are no longer needed, and you can't find any correlation between old and new code in many situations. Rather, you want to read Target.cpp of the new LLD to understand how each target is handled and mimic other architectures.

I'm happy as long as you remove unnecessary llvm:: specifier. LGTM.

Can you verify that you actually cannot dlopen() an .so file if the file has the DF_STATIC_TLS flag? I want to make sure that glibc actually uses the flag.

If the Linux kernel was the only reason you wanted -z text, and the Linux kernel doesn't actually use that flag, I'd support removing -z text flag entirely.

LGTM

LGTM

Thanks Dylan for explaining. From your explanation, I think supporting AVR is fairly easy. I'd think we probably need ~100 lines of code.

LGTM

LGTM with inlining.

I think I prefer inlining them as sequential_sort and sequential_for_each_n are in the end just sort and for_each.

These possible compatibility issues could be real concerns, but I think we should go with a simple way which is to define ehdr_start unconditionally. When we need to strike a balance among simplicity, efficiency and compatibility, we sometimes chose simplicity or efficiency over the 100% precise compatibility, and it seems to be working surprisingly well. It feels __ehdr_start is in the same category. We can add more code later if it really needs more complexity.

Do you mean the bot is still red? If I broke something I'd first try to roll it back if the error is not just a simple mistake but needs another round of code review.

Adding a byte order mark doesn't seems like a good way to fix the issue. Why don't you add a flag indicating whether it is BE or LE to convertUTF16ToUTF8String?

Right, but we already violated the assumption for symbols such as __bss_start as it is always defined even if .bss does not exist.

I'm reluctant to introduce a new concept of SymbolReference just for ehdr_start symbol which is frankly speaking not that important. You needed these new concepts for defining ehdr_start only when an ELF header is memory-allocated, right? Is there any problem if you always define that symbol and leave it zero if an ELF header is not in an allocated segment?

They were more like questions than review comments. I assumed you know much more than I about AVR, and I wanted to know more about AVR to see where AVR support would eventually go. What is your eventual goal for AVR support?

As I understand it, people do not usually run Unix-ish OS on AVR. You want to use it for baremetal programming. So, even though input files are ELF, output files are headerless binary. Naturally you don't use dynamic linking, which means most ELF features, such as PLT/GOT, are not used for AVR. Is this my understanding correct?

Hi Leslie,

• address review comments (demo)