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

[Rui Ueyama]

I'm going to check this in soon.

On Thu, May 28, 2015 at 11:08 AM, Rui Ueyama <ruiu at google.com> wrote:

> On Thu, May 28, 2015 at 10:47 AM, Rafael Espíndola <
> rafael.espindola at gmail.com> wrote:
>
>> > ================
>> > 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; }
>
>
> By doing that we can get rid of "virtual" from the function, but I think
> it slightly decreases readability of the thing. Now you have to define a
> new constructor to initialize HasData right?
>
>
>> >> 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!
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150528/5d6eac3b/attachment.html>