merge strings has bugs that will eventually crash applications built using this switch. I will add a proper implementation soon.

LGTM.

Add a test.

Thanks for providing an example. LGTM.

Can you add a test ? How do you do that in assembly to create a relocation to nothing ?

Prior to this change, the target executable writer's used to contain the TargetLinkingContexts and their TargetLayout's. The Target writers do a static cast and return their layout's which i feel is not a nice design, IMO. I think you could safely derive from OutputELFWriter and create MipsOutputELFWriter. This would save the same amount of duplicated code too. Thoughts ?

LGTM.

LGTM. Cool!

The file is a autogenerated file. But sure. I will add a comment.

LGTM.

Thanks for cleaning up.

LGTM. nice to know about this type of usage.

hopefully this is the final version :)

Implement a non-recursive version as per davide/ruiu.

I was thinking of doing it non recursive too, but I thought its easier to experiment or change the way tasks get spawned(by changing few numbers) with a recursive version, no ??

I was thinking of doing it non recursive too, but I thought its easier to experiment or change the way tasks get spawned(by changing few numbers) with a recursive version, no ??

LGTM.

this fails if the access to eh frame using eh_frame_header is absolute ?

The results are very similar to the previous patch that was posted. Regresses on performance with bigger links on my box.

Ah that was bad. Thanks for catching that.

• What is the commit message policy when reverting a change ?

Update code with comments from Rui,

I agree dividing the tasks by the hardware concurrency could get optimal performance. If you see parallel_sort(few lines above) the code tries to divide tasks by 1024. This file needs to be cleaned up based on your suggestion and I feel we could implement that later, just for allowing others to try various loops to be optimized based on this patch. I am fine anyways but I think creating an optimal solution would make others wait.

Updating the code sent for review, with my latest changes. Have a seperate constant for parallel_for_each functionality.

I agree with David's comment, it doesnot show up with any considerable improvement.

I tried to self host lld and self host clang and the time taken to link one in 10 runs, saves the link time by 1/2 a second. I changed the minParallelSize to be 512 for the function parallel_for_each to make this happen though.

I havent run benchmarks as yet with this patch as yet.

Forgot to mention about your previous change, that you need to add changes to ReaderWriterYAML(YAML Testing) and ReaderNative and WriterNative(Native formats) too.

LGTM.

am confused, Atoms live within a File and when the ordinals would be relative inside the file. So for your case do more than one file share the same SimpleDefinedAtom ? Am I missing some information ?

I dont think we need a separate visibility field in DefinedAtoms. The visibility is already modeled as part of Scope.

LGTM

LGTM.

can we split this patch by functionalities too, its easier for review.

Can you please add reviewers to the code review request next time, its possible people would miss reviewing this.

Thats a nice comparison, Ruiu. Once the YAML/Native code is changed, this LGTM. I have a usecase for section size for ELF too for internal purposes.

There are more complicated examples than that where ruleid's are required, It makes the design scale a lot, than keeping more data structures IMO.

Few comments :-

Thanks for the info, Rui. Adding items to the map while you are iterating is bad in the first place, not sure why std::map didnt show the issue. I will double check the ELF Reader/Writer for any of these occurences.

Address comments from Simon.

rnk : I was able to self-host clang/lld and run without any issues, and I would think he was trying to link chromium with the lld windows flavor.

davide: I havent measured. There are some low hanging fruits which I plan to do of resizing certain maps.

We dont have standalone build, but if we do we can pick it up from the repository.

https://llvm.org/svn/llvm-project/lld/trunk@230273

nothing from 2.o was really needed in the final object. absolute symbols are specific to a file, its easy to enhance GC to collect them IMO. This is also needed by --defsym too, if there is not a reference to that symbol, ld doesnot generate one.

There is a absolute symbol for 2.c symbol.

I feel its a bug in GNU on why absolute symbols are not reclaimed. Do we want to be bug compatible with lld ?

Can you add testcases for the other relocations that you have in the code ?

LGTM.

Filcab, you got the wrong file, please look at this file https://github.com/llvm-mirror/lld/blob/master/lib/ReaderWriter/ELF/Hexagon/HexagonEncodings.h (this is pretty common with scatter architectures, where all the information is present in the td file and you are trying to duplicate this information in target specific code in other tools, this is not clean).

https://llvm.org/svn/llvm-project/llvm/trunk@230124

Updated with comments from atanasyan.

should this be a general option in LinkingContext ? Why is this specific to the GnuFlavor ? Are you planning to add the stats collection for individual passes that run during Linkstep ?

In addition to what we discussed in this review, that was one of my intention to have a linker to support just a particular ELF target.

Can we use yaml2obj in the test ?

I disagree. This kind of code is commonly placed in the TargetHandler. I was just browsing MachO and mach-o too has these utility functions in the ArchHandler. Myself/Simon do agree that this code pattern when moved to the TargetHandler for ELF/Gnu flavor makes the code much better too.

I do agree that LinkingContext is not the place to have these functions defined. Moving this to lld::Reference is not the right thing to do IMO as its a base class for all references. We do have TargetHandler for this purpose where each target would override this functionality.

Can we also add a pointer to the specification in the code. It would be helpful for others if they happen to change this same code later.

relocation types are very much architecture dependent, thats why they are handled in each architecture/target. Its ok to move this to the TargetHandler, and the code would be more clear.

other reasons I dont like this design is