[PATCH] [LLD] Add a new PE/COFF port

[Rafael Espíndola]

> ================
> Comment at: COFF/Chunks.h:65
> @@ +64,3 @@
> +  // by this chunk is filled with zeros.
> +  virtual bool hasData() const { return true; }
> +
> ----------------
> rafael wrote:
>> Having a HasData protected member would be more llvm like.
> Writer uses this member function.

Oh, I meant in addition, so the method would look like

bool hasData() const { return HasData; }

>> This is called only for COMDAT discarding, right? There is nothing like --gc_sections at the moment.
>>
>> Where should it print? STDOUT? Document that.
> It has COMDAT merging; search for "opt:icf". And gc-sections is already implemented in this patch.

No results for icf :-)

What I mean by --gc-sections is something on the style of ELF, where
even sections that are not in COMDATs can be gced. I know link.exe
doesn't have that, I was just confused by the term "gc".


> ----------------
> rafael wrote:
>> So, make_dynamic_error_code is a bit of a misfeature IMHO.
>>
>> There are two things at play
>> * Issuing a diagnostic
>> * Returning an error
>>
>> For the diagnostic, something as simple as llvm::errs() is a reasonable start (or passing down a raw_ostream). At some point we really have move the diagnostic handling code out of lib/IR so that it can be used for all of llvm.
>>
>> For error handling, we just need as many error_code enums as the callers can handle. For example, for all of the bitcode reading we use just InvalidBitcodeSignature, CorruptedBitcode.
> I agree with that. If you want to use this as a library (not in a fancy way, but just by passing a list of mix of strings (command line options) and memory buffer objects (instead of filename string) to link in-memory objects, you want to get a machine-readable error code instead of this kind of human-readable error messages.
>
> However, there's a beauty of this dynamic_error_code thing. It contains a error string. Every time you check for an error, you can construct a new error message by appending prefix to that. For example, if you get an error string "file not found" from a subroutine, you can then construct a new error message "foo.obj: file not found" and then propagate it up to the caller. The caller may append more error messages, like "reading bar.obj's directive section: foo.obj: file not found".
>
> I don't think you can do that with error code enums.

With enums you can't, that is the point :-)

The idea is to only add an enum value if a caller wants to do
something different for that error case. That is why for bitcode
reading we have just enums for "there was an error" and "this is not a
bitcode file at all": Some callers want to try to open a file and
ignore it if it is not a bitcode, but actual errors have to be
reported.

The diagnostic path is separate and far richer:

  if (!isa<PointerType>(PtrType))
    return Error(DH, "Load/Store operand is not a pointer type");

The DH is a DiagnosticHandler callback. It can do pretty much
anything: print to stdout, abort the program, log it over the
internet, open a dialog window, etc. It is also extensible. In the
case of the bitcode reader we pass just the error_code and the user
readable message, but it is possible to include as much context as
needed.

Last but not least, the callback is an incredibly convenient point for
putting a breakpoint when debugging. When the error occurs the entire
stack is still alive.

> I'll leave this as is as this is not that important, I'll revisit this later.

I agree, please don't consider any of my comments as blockers. The
performance improvements you showed are more than convincing to get
this committed and iterate afterwards.

> rafael wrote:
>> who owns this?
> This is a placement new.

oops :-)

> rafael wrote:
>> Exposing an iterator might be better, no?
> Are you suggesting adding begin() and end() to InputFile? If so, because a subclass of thiis class contains another iterable object, vector<Chunk *>, adding begin and end is confusing.

I was thinking of creating the Chunk lazily, but I see it probably
wouldn't help much.

> ================
> Comment at: COFF/Memory.h:22
> @@ +21,3 @@
> +
> +class StringAllocator {
> +public:
> ----------------
> rafael wrote:
>> We should make this the one true StringSaver in llvm :-)
> Yeah. This is off topic, but do you think if it makes sense to add these member functions to BumpPtrAllocator itself?

I would probably leave it as a convenience class just like you have
it. It is just that the one in llvm is silly (virtual base with only
one implementation that uses a std::vector).

> As to how to handle the case you described for ELF, I guess we can change the signature of this function to return ErrorOr<SymbolBody*> and return this, Other or a new SymbolBody object. This doesn't have to be done in this patch, but this needs considering. Thank you for pointing that out.

Should work.

Thanks again for all the work on this!