LGTM

@MaskRay Please rebase and upload it with me and other people as reviewers, then I'll LGTM.

LGTM

LGTM

LGTM

I think I like sym over s as a function parameter name whose scope is not very narrow, but except that LGTM.

Yeah, I guess that showing that tip line would help users.

LGTM

LGTM

Looks good but I want someone from PPC side to take a look at this change.

LGTM

LGTM

This behavior seems to make sense but is very subtle. Can you add a comment to explain what this is doing?

Looks like this is an interesting feature, but the output might be a bit too spartan? It feels to me that it is not easy to make this feature's output into an actionable item unless you are not familiar with the debug record format. I wonder if there's an way to print out some "recommendation" to the user, say "<some class> is repeated xxx times. consider moving it out of a header file if it is written in a header.".

In D71385#1781099, @grimar wrote:

A question: do we need to specify the particular compression level in docs? My feeling is that
belongs to internal implementation, it is not really useful for user to know the level probably?

Imagine if we switch to another zlib implementation, like was suggested in the original PR,
what if it doesn't have these levels (I do not know if it has).

What would be probably better is to say something like:
"By default we use a compression, that is fast and effective enough, but if you want to
reduce the size of debug sections even more, you can specify -O2 to enable more aggressive,
but slower compression"

Sorry for asking too many questions, but how do you verify that a watermark matches the contents? Are you creating a new command?

LGTM

lld is not that Linux-centric as you implied in your comment. We have code for FreeBSD, OpenBSD, AMDGPU and Fuchsia (which isn't even a POSIX system), and adding code for NetBSD is OK and is appreciated. However, the thing we've been trying to avoid is to hard-code a platform-specific knowledge such as library paths to the linker. Combined with the fact that lld doesn't provide a way to enable/disable targets at build-time, all ld.lld executables (sometimes installed as /usr/bin/ld) work the same way no matter where they are run on. That helps those who do cross compilation a lot. That is a very appreciated property of our linker, and I don't think I want make an exception for NetBSD. Even on NetBSD, lld should behave the same as running on other kind of hosts.

LGTM

In D70919#1771198, @sidneym wrote:

Remove quotes around check-not.

-fuse-ld=lld is the correct usage. -fuse-ld=ld.lld results in an error message:
error: invalid linker name in argument '-fuse-ld=ld.lld'

• address review comment

LGTM

In D70606#1780090, @penzn wrote:

In D70606#1778998, @ruiu wrote:

Could you tell me about how you generated has_cet.exe? I wonder if there's a way to create an executable in the test instead of submitting a binary file to the repository.

It is a dummy executable (along the lines of int main() {return 0;}) built with Visual Studio 2019. This was inspired by the existing llvm-readobj PDB test which checks has_pdb.exe.

In D70606#1779258, @jhenderson wrote:

On this topic, yaml2obj has COFF support, although I don't know if that's sufficient for your use case here.

yaml2obj does not yet have CET flag support. What is the preferred path in this situation? We can add it, but it would be only used in this test.

In D71060#1779463, @russell.gallop wrote:

Here are some examples of the output from LLD. One for a llvm-tblgen ThinLTO link (-time-trace-granularity=50000) and a clang Release build (-time-trace-granularity=500). The clang link has some significant gaps in it. I'll see if I can identify where they're from and trace them.

llvm-tblgen.json52 KBDownload

clang-10.json1 KBDownload

Could you tell me about how you generated has_cet.exe? I wonder if there's a way to create an executable in the test instead of submitting a binary file to the repository.

As I understand it, the scenario is:

1. Do link; 2) Run the watermark tool to append .note.llvm.watermark; 3) Release SDK; 4) Downstream vendors modify .data and ship to end users; 5) End users verify that .note.llvm.watermark does not match computed watermark of loadable contents.
   
   The build process before 3) are all controlled. The process should ensure there is no modification to .data between 1) and 2). How do you guarantee a linker side feature can prevent modification? How can you prevent the following:
2. Do link and generate .note.llvm.watermark in one step 1.5) Modify .data 2) Run the watermark tool to update .note.llvm.watermark

This is a motivation to not have an external tool on the watermarking. That being said, as @chrisjackson has said on more than one occasion, this isn't intended to be a security feature so we are not attempting to detect a malicious attacker. It could be possible for downstream LLD producers to add a local salt to the watermark to make it more secure, should they so choose, I suppose.

LGTM

LGTM

Can you share an example output of lld with this patch?

Thank you for fixing the issue. This is an interesting edge case that I haven't thought about that before.

Hmm, I'm sorry but I'm confused. IIRC I had a discussion in the LLVM dev meeting that we were going to submit a change with a single PLT scheme rather than IPLT, so when I said that I'm going to submit a patch, I meant that I'm going to submit a patch for the 1PLT scheme rather than the 2PLT scheme. But this is for the 2PLT scheme. This is not something I want.

In D71242#1778777, @MaskRay wrote:

In D71242#1778767, @ruiu wrote:

classof is an LLVM's way of implementing a dynamic casting, and the code is idiomatic -- we almost always dispatch only by an enum field. So this could be a bit confusing to those who are expecting that idiomatic mechanism? I wonder if it is better to define a new function instead of overloading classof.

We already have such a code pattern

class EhFrameSection final : public SyntheticSection {
public:
  EhFrameSection();
  void writeTo(uint8_t *buf) override;
  void finalizeContents() override;
  bool isNeeded() const override { return !sections.empty(); }
  size_t getSize() const override { return size; }

  static bool classof(const SectionBase *d) {
    return SyntheticSection::classof(d) && d->name == ".eh_frame";
  }

classof is an LLVM's way of implementing a dynamic casting, and the code is idiomatic -- we almost always dispatch only by an enum field. So this could be a bit confusing to those who are expecting that idiomatic mechanism? I wonder if it is better to define a new function instead of overloading classof.

Aren't Parallel and ThreadPool different? We are using parallel_for_loop but we are not using any thread pool.

Sure, let me submit this patch for you.

I still feel that I prefer the previous code, but that's not a strong preference, so I'm fine with this.

LGTM

LGTM with these fixes.

Nope, if you have a commit access, can you commit this?

LGTM

LGTM

LGTM

James Knight has submitted a very similar change to fix the problem.

Thank you for the patch. Overall, I think it is a good thing to add link.exe-compatible /map file output to lld.

LGTM

In D70505#1754462, @dblaikie wrote:

In D70505#1754388, @ruiu wrote:

This is my personal taste, but I feel like the former code is simpler, though it doesn't look elegant. The new code looks a bit too smart to me. I also have a little concern to define start and end to SymbolTable class because it is not very clear what are iterated. SymbolTable contains not only symbols but other members.

Would it alleviate your concerns if it were more like:

for (Symbol *sym : symtab->symbols())

?

This is my personal taste, but I feel like the former code is simpler, though it doesn't look elegant. The new code looks a bit too smart to me. I also have a little concern to define start and end to SymbolTable class because it is not very clear what are iterated. SymbolTable contains not only symbols but other members.

Since you have moved all the files to a separate directory, it is probably better to create README.md to explain what Propeller is and how it works. You may want to move some file comments to that file.

LGTM

In D70421#1751289, @grimar wrote:

Does it make sense to accumulate all global variables in the one place? Like place them to a separate file with initGlobals method?
I think one of their problems is that they spreaded everywhere and initialized from many places.

I think this feature is worth more attention. There may be someone who wants to start using this once it's landed, and I'd make sure that we satisfy their needs. Do you mind if I ask you to start a thread on llvm-dev to propose this feature? I think that even a comment like "we'll use this" is valuable. tlike "

I think I agree with dalias. I don't get a point to allow users to use copy relocations liberally for a new ISA/ABI like RISC-V, and it looks like it is a problem that we should fix compilers as soon as possible, rather than adopting the linker to what compilers emit at the moment. Even though I can see the reason why you wanted to add this to lld, I can also imagine that that would be a regrettable decision in the long term.

In D70399#1752600, @MaskRay wrote:

I agree that making the field an enumeration is an improvement, but I also feel that the closing bracket ] on a different line is a bit verbose.

Ideally we can make it on one line, e.g. Flags [], Flags [ Info (0x4), 0x8] (0x8 is an unspecified bit field. Bikeshedding on the representation welcomed!)

The other way of testing this is actually invoking the linker twice from main(), only in a test mode (specified by some environment variable), to see if it works. It is doable, and it should be able to detect all errors, but I'm not sure if we want to run tests twice.

James, I'm sorry that I think I missed your second comment. So if gcc and clang uses LE model even for extern TLS variables, I agree with you that it's odd. They may have wanted to avoid the cost of more general access models, but because the linker is able to relax them, that shouldn't be a big problem.

LGTM

I share your concern and I'm sorry about leaving so many global variables as uninitialized in the second run of the linker in the same process. But I'd like to find and fix the problems without abolishing global variables in lld, as I don't actually dislike them. One thing we could do is initializing global variables with invalid data -- like -1 or {nullptr} so that you'd get an error even on the first run if you do not reset them before use. I'll create a patch and send it to you.

Copy relocation is a hack to allow non-PIC object files to be linked with DSOs. And it is a bit dangerous hack because people don't usually understand the exact semantics of the copy relocation and can make an ABI-breaking change without knowing it. I described the problem here: https://github.com/llvm/llvm-project/blob/master/lld/ELF/Relocations.cpp#L517-L558