[PATCH] D35961: [llvm] Update MachOObjectFile::exports interface

[Kevin Enderby via cfe-commits]

Looks fine to me.

Kev

> On Jul 28, 2017, at 3:49 PM, Alexander Shaposhnikov <alexander.v.shaposhnikov at gmail.com> wrote:
> 
> Hi, 
> many thanks for looking at the diff.
> (i started working on this because this interface change broke some out-of-tree code, but that's expected and not a big issue, i just wanted to clean it up a bit). 
> 
> I assume I might be missing smth, 
> however my diff doesn't change the static method  
> /// For use examining a trie not in a MachOObjectFile.
> static iterator_range<export_iterator> exports(Error &Err,
>                                                  ArrayRef<uint8_t> Trie,
>                                                  const MachOObjectFile *O =
>                                                                       nullptr);
> 
> It changes only the regular method  
> /// For use iterating over all exported symbols.
> iterator_range<export_iterator> exports(Error &Err, const MachOObjectFile *O) const;
> because I didn't like syntax Obj->exports(Err, Obj).
> LLD builds successfully with my patch - so I am wondering if this change looks reasonable/safe to you.
>  
> Thanks,
> Alex Shaposhnikov
> 
> 
> 
> On Fri, Jul 28, 2017 at 10:30 AM, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
> 
> 
> On Fri, Jul 28, 2017 at 10:24 AM Kevin Enderby <enderby at apple.com <mailto:enderby at apple.com>> wrote:
>> On Jul 27, 2017, at 9:50 PM, Saleem Abdulrasool via Phabricator <reviews at reviews.llvm.org <mailto:reviews at reviews.llvm.org>> wrote:
>> 
>> compnerd added a comment.
>> 
>> Does anyone use the overload with the `O` for `exports` with `nullptr` instead of `this`?  If not, we could just inline `this` throughout.
> 
> This will break lld as it needs the default nullptr.  See r308691.
> 
> The reason that O was added was so that this check from r308690 could be added.
> 
> When O is specified is it always == this, though? That seems to be implied by the original post.
> 
> If that's the case, then the ability to pass null is really only passing the on/off state for the diagnostic? Is that necessary, or would it be good to just always produce the Error, even in lld?
>  
> 
> +      if (O != nullptr) {
> +        if (State.Other > O->getLibraryCount()) {
> +          *E = malformedError("bad library ordinal: " + Twine((int)State.Other)
> +               + " (max " + Twine((int)O->getLibraryCount()) + ") in export "
> +               "trie data at node: 0x" + utohexstr(offset));
> +          moveToEnd();
> +          return;
> +        }
> 
> This is needed for the test case:
> 
> +RUN: not llvm-objdump -macho -exports-trie %p/Inputs/macho-trie-bad-library-ordinal 2>&1 | FileCheck -check-prefix BAD_LIBRARY_ORDINAL %s 
> +BAD_LIBRARY_ORDINAL: macho-trie-bad-library-ordinal': truncated or malformed object (bad library ordinal: 69 (max 3) in export trie data at node: 0x33)
> 
>> 
>> 
>> 
>> ================
>> Comment at: tools/llvm-nm/llvm-nm.cpp:1230
>>       Error Err = Error::success();
>> -      for (const llvm::object::ExportEntry &Entry : MachO->exports(Err,
>> -                                                                   MachO)) {
>> +      for (const llvm::object::ExportEntry &Entry : MachO->exports(Err)) {
>>         bool found = false;
>> ----------------
>> I think that using `auto` here instead of `llvm::object:ExportEntry` is better for readability.
>> 
>> 
>> ================
>> Comment at: tools/llvm-objdump/MachODump.cpp:9406
>>   Error Err = Error::success();
>> -  for (const llvm::object::ExportEntry &Entry : Obj->exports(Err, Obj)) {
>> +  for (const llvm::object::ExportEntry &Entry : Obj->exports(Err)) {
>>     uint64_t Flags = Entry.flags();
>> ----------------
>> Similar.
>> 
>> 
>> Repository:
>>  rL LLVM
>> 
>> https://reviews.llvm.org/D35961 <https://reviews.llvm.org/D35961>
>> 
>> 
>> 
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170728/b7864123/attachment.html>