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

[Rui Ueyama]

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/07ce1efb/attachment.html>