Re: [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 
>  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 exports(Error ,
>  ArrayRef Trie,
>  const MachOObjectFile *O =
>   
> nullptr);
> 
> It changes only the regular method  
> /// For use iterating over all exported symbols.
> iterator_range exports(Error , 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  > wrote:
> 
> 
> On Fri, Jul 28, 2017 at 10:24 AM Kevin Enderby  > wrote:
>> On Jul 27, 2017, at 9:50 PM, Saleem Abdulrasool via Phabricator 
>> > 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  : MachO->exports(Err,
>> -   MachO)) {
>> +  for (const llvm::object::ExportEntry  : 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  : Obj->exports(Err, Obj)) {
>> +  for (const llvm::object::ExportEntry  : Obj->exports(Err)) {
>> uint64_t Flags = Entry.flags();
>> 
>> Similar.
>> 
>> 
>> Repository:
>>  rL LLVM
>> 
>> https://reviews.llvm.org/D35961 
>> 
>> 
>> 
> 
> 

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kevin Enderby via cfe-commits]

> On Jul 28, 2017, at 10:30 AM, David Blaikie  wrote:
> 
> 
> 
> On Fri, Jul 28, 2017 at 10:24 AM Kevin Enderby  > wrote:
>> On Jul 27, 2017, at 9:50 PM, Saleem Abdulrasool via Phabricator 
>> > 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?

Without O one can’t use O->getLibraryCount() which is what is needed for the 
test.  I guess one could change the interface if lld really wanted this check. 
But I’d leave that to the lld folks as to what level of error checking they 
want.

>  
> 
> +  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  : MachO->exports(Err,
>> -   MachO)) {
>> +  for (const llvm::object::ExportEntry  : 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  : Obj->exports(Err, Obj)) {
>> +  for (const llvm::object::ExportEntry  : Obj->exports(Err)) {
>> uint64_t Flags = Entry.flags();
>> 
>> Similar.
>> 
>> 
>> Repository:
>>  rL LLVM
>> 
>> https://reviews.llvm.org/D35961 
>> 
>> 
>> 
> 

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kevin Enderby via cfe-commits]

> On Jul 27, 2017, at 9:50 PM, Saleem Abdulrasool via Phabricator 
>  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.

+  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  : MachO->exports(Err,
> -   MachO)) {
> +  for (const llvm::object::ExportEntry  : 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  : Obj->exports(Err, Obj)) {
> +  for (const llvm::object::ExportEntry  : Obj->exports(Err)) {
> uint64_t Flags = Entry.flags();
> 
> Similar.
> 
> 
> Repository:
>  rL LLVM
> 
> https://reviews.llvm.org/D35961
> 
> 
> 

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits