<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, May 28, 2015 at 10:47 AM, Rafael Espíndola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>> ================<br>
> Comment at: COFF/Chunks.h:65<br>
> @@ +64,3 @@<br>
> +  // by this chunk is filled with zeros.<br>
> +  virtual bool hasData() const { return true; }<br>
> +<br>
> ----------------<br>
> rafael wrote:<br>
>> Having a HasData protected member would be more llvm like.<br>
> Writer uses this member function.<br>
<br>
</span>Oh, I meant in addition, so the method would look like<br>
<br>
bool hasData() const { return HasData; }</blockquote><div><br></div><div>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?</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span><br>
>> This is called only for COMDAT discarding, right? There is nothing like --gc_sections at the moment.<br>
>><br>
>> Where should it print? STDOUT? Document that.<br>
> It has COMDAT merging; search for "opt:icf". And gc-sections is already implemented in this patch.<br>
<br>
</span>No results for icf :-)<br>
<br>
What I mean by --gc-sections is something on the style of ELF, where<br>
even sections that are not in COMDATs can be gced. I know link.exe<br>
doesn't have that, I was just confused by the term "gc".<br>
<span><br>
<br>
> ----------------<br>
> rafael wrote:<br>
>> So, make_dynamic_error_code is a bit of a misfeature IMHO.<br>
>><br>
>> There are two things at play<br>
>> * Issuing a diagnostic<br>
>> * Returning an error<br>
>><br>
>> 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.<br>
>><br>
>> 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.<br>
> 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.<br>
><br>
> 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".<br>
><br>
> I don't think you can do that with error code enums.<br>
<br>
</span>With enums you can't, that is the point :-)<br>
<br>
The idea is to only add an enum value if a caller wants to do<br>
something different for that error case. That is why for bitcode<br>
reading we have just enums for "there was an error" and "this is not a<br>
bitcode file at all": Some callers want to try to open a file and<br>
ignore it if it is not a bitcode, but actual errors have to be<br>
reported.<br>
<br>
The diagnostic path is separate and far richer:<br>
<br>
  if (!isa<PointerType>(PtrType))<br>
    return Error(DH, "Load/Store operand is not a pointer type");<br>
<br>
The DH is a DiagnosticHandler callback. It can do pretty much<br>
anything: print to stdout, abort the program, log it over the<br>
internet, open a dialog window, etc. It is also extensible. In the<br>
case of the bitcode reader we pass just the error_code and the user<br>
readable message, but it is possible to include as much context as<br>
needed.<br>
<br>
Last but not least, the callback is an incredibly convenient point for<br>
putting a breakpoint when debugging. When the error occurs the entire<br>
stack is still alive.<br>
<span><br>
> I'll leave this as is as this is not that important, I'll revisit this later.<br>
<br>
</span>I agree, please don't consider any of my comments as blockers. The<br>
performance improvements you showed are more than convincing to get<br>
this committed and iterate afterwards.<br>
<span><br>
> rafael wrote:<br>
>> who owns this?<br>
> This is a placement new.<br>
<br>
</span>oops :-)<br>
<span><br>
> rafael wrote:<br>
>> Exposing an iterator might be better, no?<br>
> 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.<br>
<br>
</span>I was thinking of creating the Chunk lazily, but I see it probably<br>
wouldn't help much.</blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span><br>
> ================<br>
> Comment at: COFF/Memory.h:22<br>
> @@ +21,3 @@<br>
> +<br>
> +class StringAllocator {<br>
> +public:<br>
> ----------------<br>
> rafael wrote:<br>
>> We should make this the one true StringSaver in llvm :-)<br>
> Yeah. This is off topic, but do you think if it makes sense to add these member functions to BumpPtrAllocator itself?<br>
<br>
</span>I would probably leave it as a convenience class just like you have<br>
it. It is just that the one in llvm is silly (virtual base with only<br>
one implementation that uses a std::vector).<br>
<span><br>
> 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.<br>
<br>
</span>Should work.<br>
<br>
Thanks again for all the work on this!<br>
</blockquote></div><br></div></div>