llvm dev - Jun 2012 - [LLVMdev] object construction patterns and unique_ptr

On Jun 16, 2012, at 3:51 PM, Chris Lattner wrote:
> On Jun 15, 2012, at 3:48 PM, Nick Kledzik wrote:
> 
>> In lld we have a Reader class which is a factory that takes .o file(s)
and produces an in memory lld::File object(s).  But in doing so, there could be
I/O errors (file not readable) or file may be malformed.  We are also using
C++11 in lld, so we use std::unique_ptr for managing object ownership.
>> 
>> The Reader class currently has an interface that can be simplified down
to:
>> 
>>   virtual error_code readFile(StringRef path,
std::unique_ptr<lld::File> &result);
>> 
>> But this interface has become awkward to use. There are two
"return" values.  This method is a unique_ptr "source" but
the use of a by-refernce parameter means you have to start with an uninitialized
unique_ptr and the readFile() has to std::move() into it.   unique_ptr is much
nicer to use as a return value.
> 
> Not a c++'11 expert here, but does a returning an std::tuple work?
I talked to Howard about that.  It does work, but it is cumbersome.  Either:

std::tuple<std::unique_ptr<lld::File>, error_code>  result =
reader.readFile(path);

Or you use std::tie() but then you still have to declare the two local variables
and tie them together.  You could define a wrapper class for the tuple, but then
you realize it would be simpler to push the error_code into the File and get rid
of the wrapper class.
>> An other issue is that since llvm::error_code was designed to return a
fixed set of strings, there is no way to return a dynamic error message (e.g.
"relocation 27 is invalid").
> 
> I'm pretty sure that the linker can specify a custom error_category
with whatever information you want, and then return an error_code put with that.
Michael would know for sure, but it is definitely in the design of error_code
for it to be extensible.
>From what I can tell error_code is about wrapping out OS specific
"int" error codes, and how to map those to static strings.  I suppose
you could abuse it by having a error_category which has a static vector or
recently dynamically generated strings and the int "error code" is an
index into that vector.
Clang has this great Diagnostics infrastructure for showing where the error
occurred in the source file.  Does developing something like that make sense for
object files whose source is not human readable?
>> In asking around for clean models on how to do this better, I've
heard two interesting ideas:
>> 
>> 1) Incorporate the error_code and string message into the File class. 
That is the factory method always returns a File object. But there is a method
on File objects to query if they are valid or not.  If they are not, there is a
method to get the error_code and/or error string.
>> 
>> 2) A variation on this is to have a new subclass of File called
FileError.  If the Reader cannot construct a real FileFoo object because of some
error, it instead constructs a FileError object and returns that.  The FileError
object has methods to get the error_code and/or error string.
>> 
>> With both these ideas the Reader method is:
>> 
>>   virtual std::unique_ptr<lld::File> readFile(StringRef path);
>> 
>> which is much easier to use.
>> 
>> 
>> Any recommendations on a good way to design this?
> 
> Would an API like:
> 
>   virtual std::unique_ptr<lld::File> readFile(StringRef path,
error_code &ErrorInfo);
That is a common pattern, used for instance in the bit code reader.  If we can
unify the OS level error and the syntax/semantic errors from parsing object
files, this pattern might work.

One other thought that leans me towards integrating the error code/msg into the
File object is that it works cleaner when you start adding multithreading to the
linker.  When you have multiple threads going, each reading an object file and
one of them returns an error, you need to cleaning shut down the other threads,
and return the error.  With the error status integrated into the File object,
all you do is have each thread append it resulting File object onto a list, then
after all threads are joined, run through the list once to see if any say they
encountered an error.  If the error_code is returned separately from the File,
you have to create some data structure to bind together the error_code with
which file path could not be parsed.

-Nick

On Mon, Jun 18, 2012 at 1:22 PM, Nick Kledzik <kledzik at apple.com>
wrote:
> On Jun 16, 2012, at 3:51 PM, Chris Lattner wrote:
>> On Jun 15, 2012, at 3:48 PM, Nick Kledzik wrote:
>>
>>> In lld we have a Reader class which is a factory that takes .o
file(s) and produces an in memory lld::File object(s).  But in doing so, there
could be I/O errors (file not readable) or file may be malformed.  We are also
using C++11 in lld, so we use std::unique_ptr for managing object ownership.
>>>
>>> The Reader class currently has an interface that can be simplified
down to:
>>>
>>>   virtual error_code readFile(StringRef path,
std::unique_ptr<lld::File> &result);
>>>
>>> But this interface has become awkward to use. There are two
"return" values.  This method is a unique_ptr "source" but
the use of a by-refernce parameter means you have to start with an uninitialized
unique_ptr and the readFile() has to std::move() into it.   unique_ptr is much
nicer to use as a return value.
>>
>> Not a c++'11 expert here, but does a returning an std::tuple work?
> I talked to Howard about that.  It does work, but it is cumbersome.
 Either:
>
> std::tuple<std::unique_ptr<lld::File>, error_code>  result =
reader.readFile(path);
>
> Or you use std::tie() but then you still have to declare the two local
variables and tie them together.  You could define a wrapper class for the
tuple, but then you realize it would be simpler to push the error_code into the
File and get rid of the wrapper class.
>
>>> An other issue is that since llvm::error_code was designed to
return a fixed set of strings, there is no way to return a dynamic error message
(e.g. "relocation 27 is invalid").
>>
>> I'm pretty sure that the linker can specify a custom error_category
with whatever information you want, and then return an error_code put with that.
 Michael would know for sure, but it is definitely in the design of error_code
for it to be extensible.
>
> >From what I can tell error_code is about wrapping out OS specific
"int" error codes, and how to map those to static strings.  I suppose
you could abuse it by having a error_category which has a static vector or
recently dynamically generated strings and the int "error code" is an
index into that vector.
>
> Clang has this great Diagnostics infrastructure for showing where the error
occurred in the source file.  Does developing something like that make sense for
object files whose source is not human readable?
The chance that a user will get an invalid object file is incredibly
low, and if they do get one there's generally little they can do about
it. However when this does happen we need to be able to figure out
what's going on. So we do not need clang style diagnostic printing for
object file parsing, but we do still need to get useful diagnostics.
>>> In asking around for clean models on how to do this better,
I've heard two interesting ideas:
>>>
>>> 1) Incorporate the error_code and string message into the File
class.  That is the factory method always returns a File object. But there is a
method on File objects to query if they are valid or not.  If they are not,
there is a method to get the error_code and/or error string.
I like this with some modifications.
>>> 2) A variation on this is to have a new subclass of File called
FileError.  If the Reader cannot construct a real FileFoo object because of some
error, it instead constructs a FileError object and returns that.  The FileError
object has methods to get the error_code and/or error string.
I dislike this variant because a FileError is not a File.
>>> With both these ideas the Reader method is:
>>>
>>>   virtual std::unique_ptr<lld::File> readFile(StringRef
path);
>>>
>>> which is much easier to use.
>>>
>>>
>>> Any recommendations on a good way to design this?
>>
>> Would an API like:
>>
>>   virtual std::unique_ptr<lld::File> readFile(StringRef path,
error_code &ErrorInfo);
>
> That is a common pattern, used for instance in the bit code reader.  If we
can unify the OS level error and the syntax/semantic errors from parsing object
files, this pattern might work.
>
> One other thought that leans me towards integrating the error code/msg into
the File object is that it works cleaner when you start adding multithreading to
the linker.  When you have multiple threads going, each reading an object file
and one of them returns an error, you need to cleaning shut down the other
threads, and return the error.  With the error status integrated into the File
object, all you do is have each thread append it resulting File object onto a
list, then after all threads are joined, run through the list once to see if any
say they encountered an error.  If the error_code is returned separately from
the File, you have to create some data structure to bind together the error_code
with which file path could not be parsed.
>
> -Nick
I believe that specifically for object file parsing errors a
pair<error_code, vector<string> values> (or something similar) as a
member of File would work. This would be used as:

// In the error_category stuff
class blah_reader_error_category : public llvm::_do_message {
public:
  virtual std::string message(int ev) const {
    ...
    case uknown_reloc:
      return "uknown relocation % at %";
    ...
  }
};

// In the reader.

setError(make_pair(blah_error::unkown_reloc,
vector<string>{relocationumer, loc})); // loc could be calculated by
backtracking, or just the current file address. (we could even pass
the address as a stop address to obj2yaml).

// Someplace else.

if (File->error())
  o << File->getErrorMessage();

Which would do the string substitution.

This lets us not spend any time building up a representation for
diagnostics when none will be printed, and still get decent
diagnostics with well defined enum values when needed.

So:

virtual std::unique_ptr<lld::File> readFile(StringRef path);

class File {
public:
  bool error();
  error_code getError();
  string getErrorMessage();
protected:
  void setError(pair<error_code, vector<string>>);
private:
  pair<error_code, vector<string>> Error;
}

I believe we may also want to tablegen the different errors so it's
trivial to add a new one.

Note that for proper linker errors (undefined reference etc...) I
believe we need something more extensive.

- Michael Spencer

On Jun 18, 2012, at 1:22 PM, Nick Kledzik wrote:
>>> But this interface has become awkward to use. There are two
"return" values.  This method is a unique_ptr "source" but
the use of a by-refernce parameter means you have to start with an uninitialized
unique_ptr and the readFile() has to std::move() into it.   unique_ptr is much
nicer to use as a return value.
>> 
>> Not a c++'11 expert here, but does a returning an std::tuple work?
> I talked to Howard about that.  It does work, but it is cumbersome. 
Either:
> 
> std::tuple<std::unique_ptr<lld::File>, error_code>  result =
reader.readFile(path);
> 
> Or you use std::tie() but then you still have to declare the two local
variables and tie them together.  You could define a wrapper class for the
tuple, but then you realize it would be simpler to push the error_code into the
File and get rid of the wrapper class.
Ok, yeah, that is annoying.
>>> An other issue is that since llvm::error_code was designed to
return a fixed set of strings, there is no way to return a dynamic error message
(e.g. "relocation 27 is invalid").
>> 
>> I'm pretty sure that the linker can specify a custom error_category
with whatever information you want, and then return an error_code put with that.
Michael would know for sure, but it is definitely in the design of error_code
for it to be extensible.
> 
> From what I can tell error_code is about wrapping out OS specific
"int" error codes, and how to map those to static strings.  I suppose
you could abuse it by having a error_category which has a static vector or
recently dynamically generated strings and the int "error code" is an
index into that vector.
I'm pretty sure that error_code is designed to be "simple for
errno's" but "general to other errors".  Michael?
> Clang has this great Diagnostics infrastructure for showing where the error
occurred in the source file.  Does developing something like that make sense for
object files whose source is not human readable?
I don't think this makes sense for the linker.  The key aspect of
Clang's diagnostics (and llvm::SourceMgr, which is similar but simpler) is
that it points back into source code.  This is the source of most of the
complexity of its diagnostics stuff, and doesn't make a lot of sense for the
linker.

That said, one crazy hack that I've been wanting to do for some time in
clang would apply equally well to the linker: all the command line options could
be plopped into a text buffer, and diagnostics related to command line options
could refer to *that* location.  Imagine getting an error like this from the
linker:

zsh> ld foo.o bar.o baz.a -o a.out
command-line:1:3: error: foo.o is not a valid object file
ld foo.o bar.o baz.a -o a.out
   ^~~~~

zsh> ld foo.o bar.o baz.a -o a.out
command-line:1:32: error: xyz.o member of baz.a is not a valid object file
ld foo.o bar.o baz.a -o a.out
               ^~~~~

zsh> ld foo.o bar.o baz.a -o a.out
command-line:1:46: error: can not open output file a.out: permission error
ld foo.o bar.o baz.a -o a.out
                        ^~~~~
or whatever.
>From an API level, this would mean that the object file parsing logic would
pass back up an error_code, and then the higher level driver logic would combine
the string together with the location of the command line option for printing
through llvm::SourceMgr.
> One other thought that leans me towards integrating the error code/msg into
the File object is that it works cleaner when you start adding multithreading to
the linker.  When you have multiple threads going, each reading an object file
and one of them returns an error, you need to cleaning shut down the other
threads, and return the error.  With the error status integrated into the File
object, all you do is have each thread append it resulting File object onto a
list, then after all threads are joined, run through the list once to see if any
say they encountered an error.  If the error_code is returned separately from
the File, you have to create some data structure to bind together the error_code
with which file path could not be parsed.
As with the other guys, I'd really prefer to avoid merging the error status
*into* the file.  In principle, it is the "open file" operation that
can fail, not the file itself.

-Chris

snip

Forgive me if I am way off base, but if this is a "C++11" library why
can't
exceptions be used to signal the error?

Nathan
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120619/b0c724b4/attachment.html>

On Jun 18, 2012 1:24 PM, "Nick Kledzik" <kledzik at apple.com>
wrote:
>
> On Jun 16, 2012, at 3:51 PM, Chris Lattner wrote:
> > On Jun 15, 2012, at 3:48 PM, Nick Kledzik wrote:
> >
> >> In lld we have a Reader class which is a factory that takes .o
file(s)
and produces an in memory lld::File object(s).  But in doing so, there
could be I/O errors (file not readable) or file may be malformed.  We are
also using C++11 in lld, so we use std::unique_ptr for managing object
ownership.
> >>
> >> The Reader class currently has an interface that can be simplified
down to:
> >>
> >>   virtual error_code readFile(StringRef path,
std::unique_ptr<lld::File> &result);
> >>
> >> But this interface has become awkward to use. There are two
"return"
values.  This method is a unique_ptr "source" but the use of a
by-refernce
parameter means you have to start with an uninitialized unique_ptr and the
readFile() has to std::move() into it.   unique_ptr is much nicer to use as
a return value.
> >
> > Not a c++'11 expert here, but does a returning an std::tuple work?
> I talked to Howard about that.  It does work, but it is cumbersome.
 Either:
>
> std::tuple<std::unique_ptr<lld::File>, error_code>  result
reader.readFile(path);
>
I suggest an error_or<T> class that can be combined with any result type,
not just File. It could be implemented with a fully generic variant class,
but the interface would likely be better with some specialization to
errors.
> Or you use std::tie() but then you still have to declare the two local
variables and tie them together.  You could define a wrapper class for the
tuple, but then you realize it would be simpler to push the error_code into
the File and get rid of the wrapper class.
>
> >> An other issue is that since llvm::error_code was designed to
return a
fixed set of strings, there is no way to return a dynamic error message
(e.g. "relocation 27 is invalid").
> >
> > I'm pretty sure that the linker can specify a custom
error_category
with whatever information you want, and then return an error_code put with
that.  Michael would know for sure, but it is definitely in the design of
error_code for it to be extensible.
>
> >From what I can tell error_code is about wrapping out OS specific
"int"
error codes, and how to map those to static strings.  I suppose you could
abuse it by having a error_category which has a static vector or recently
dynamically generated strings and the int "error code" is an index
into
that vector.
>
> Clang has this great Diagnostics infrastructure for showing where the
error occurred in the source file.  Does developing something like that
make sense for object files whose source is not human
readable?
>
> >> In asking around for clean models on how to do this better,
I've heard
two interesting ideas:
> >>
> >> 1) Incorporate the error_code and string message into the File
class.
 That is the factory method always returns a File object. But there is a
method on File objects to query if they are valid or not.  If they are not,
there is a method to get the error_code and/or error
string.
> >>
> >> 2) A variation on this is to have a new subclass of File called
FileError.  If the Reader cannot construct a real FileFoo object because of
some error, it instead constructs a FileError object and returns that.  The
FileError object has methods to get the error_code and/or error
string.
> >>
> >> With both these ideas the Reader method is:
> >>
> >>   virtual std::unique_ptr<lld::File> readFile(StringRef
path);
> >>
> >> which is much easier to use.
> >>
> >>
> >> Any recommendations on a good way to design this?
> >
> > Would an API like:
> >
> >   virtual std::unique_ptr<lld::File> readFile(StringRef path,
error_code &ErrorInfo);
>
> That is a common pattern, used for instance in the bit code reader.  If
we can unify the OS level error and the syntax/semantic errors from parsing
object files, this pattern might work.
>
> One other thought that leans me towards integrating the error code/msg
into the File object is that it works cleaner when you start adding
multithreading to the linker.  When you have multiple threads going, each
reading an object file and one of them returns an error, you need to
cleaning shut down the other threads, and return the error.  With the error
status integrated into the File object, all you do is have each thread
append it resulting File object onto a list, then after all threads are
joined, run through the list once to see if any say they encountered an
error.  If the error_code is returned separately from the File, you have to
create some data structure to bind together the error_code with which file
path could not be parsed.
>
> -Nick
>
>
>
>
>
>
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120619/6d263d3f/attachment.html>

On Jun 18, 2012, at 4:36 PM, Michael Spencer wrote:
>>>> 
>>>> In asking around for clean models on how to do this better,
I've heard two interesting ideas:
>>>> 
>>>> 1) Incorporate the error_code and string message into the File
class.  That is the factory method always returns a File object. But there is a
method on File objects to query if they are valid or not.  If they are not,
there is a method to get the error_code and/or error string.
> 
> I like this with some modifications.
> 
>>>> 2) A variation on this is to have a new subclass of File called
FileError.  If the Reader cannot construct a real FileFoo object because of some
error, it instead constructs a FileError object and returns that.  The FileError
object has methods to get the error_code and/or error string.
> 
My intuition is the opposite here.  If you add the error information to the
lld::File base class then leads you to write the constructor for FileFoo() which
does all the parsing, and if it runs into a problem, just sets the error ivar
and returns (your FileCOFF is an example of that).  The problem this leads to is
half constructed objects.  That can trip up a client which might accidentally
call methods on the half constructed object  other than the error getting
methods.  It also means the destructor has to be careful when tearing down the
half constructed object.

On the other hand making a new subclass of lld::File called lld::FileError leads
you to a different style of programming.  You have to open and parse the file
past the point that any error can occur before you construct the FileFoo object.
If anything goes wrong up to the point, you instead construct a FileError object
and return that.
> I dislike this variant because a FileError is not a File.
I'm not sure what you mean by "not a File".  It is a subclass of
lld::File.  Its path() method returns the file system path that tried to be
used.

Think of it as a proxy.  The real file could not parsed.  It represents that
state.

> 
>>>> With both these ideas the Reader method is:
>>>> 
>>>>   virtual std::unique_ptr<lld::File> readFile(StringRef
path);
>>>> 
>>>> which is much easier to use.
>>>> 
>>>> 
>>>> Any recommendations on a good way to design this?
>>> 
>>> Would an API like:
>>> 
>>>   virtual std::unique_ptr<lld::File> readFile(StringRef path,
error_code &ErrorInfo);
>> 
>> That is a common pattern, used for instance in the bit code reader.  If
we can unify the OS level error and the syntax/semantic errors from parsing
object files, this pattern might work.
>> 
>> One other thought that leans me towards integrating the error code/msg
into the File object is that it works cleaner when you start adding
multithreading to the linker.  When you have multiple threads going, each
reading an object file and one of them returns an error, you need to cleaning
shut down the other threads, and return the error.  With the error status
integrated into the File object, all you do is have each thread append it
resulting File object onto a list, then after all threads are joined, run
through the list once to see if any say they encountered an error.  If the
error_code is returned separately from the File, you have to create some data
structure to bind together the error_code with which file path could not be
parsed.
>> 
>> -Nick
> 
> I believe that specifically for object file parsing errors a
> pair<error_code, vector<string> values> (or something similar)
as a
> member of File would work. This would be used as:
> 
> // In the error_category stuff
> class blah_reader_error_category : public llvm::_do_message {
> public:
>  virtual std::string message(int ev) const {
>    ...
>    case uknown_reloc:
>      return "uknown relocation % at %";
>    ...
>  }
> };
> 
> // In the reader.
> 
> setError(make_pair(blah_error::unkown_reloc,
> vector<string>{relocationumer, loc})); // loc could be calculated by
> backtracking, or just the current file address. (we could even pass
> the address as a stop address to obj2yaml).
> 
> // Someplace else.
> 
> if (File->error())
>  o << File->getErrorMessage();
> 
> Which would do the string substitution.
What is the advantage of this delayed substitution?  The site creating the error
needs to convert all values to strings. Your example has two numbers (reloc
value and address) which would need to be converted to strings.  It would be
simpler all around to just generate the complete string right there at the error
site.  Are you trying to support localization? or error numbers?

Also, are these enum values per file format (i.e. does ELF and mach-o each
define their own set)?  Or is there one big set of enums for use by all Readers?
> 
> This lets us not spend any time building up a representation for
> diagnostics when none will be printed, and still get decent
> diagnostics with well defined enum values when needed.
What is the rule of thumb of when a enum value is needed (as opposed to
success/failure)?  My thinking is that an enum only makes sense if the client
can do something about a specific enum value.  For instance in some future IDE,
maybe file_not_found error can prod the IDE to recompile something to regenerate
the object file.  Another reason for enums is if you want a way to localize
error strings to different languages.  The enums are an index into a
per-localization string table.  Given that these error (e.g. unknown relocation)
are a bug in the compiler or linker and not a problem the user of the tools can
remedy, I'm not sure they need localizing.


-Nick

On Tue, Jun 19, 2012 at 11:18 AM, Jeffrey Yasskin <jyasskin at
google.com>wrote:
> On Jun 18, 2012 1:24 PM, "Nick Kledzik" <kledzik at
apple.com> wrote:
> >
> > On Jun 16, 2012, at 3:51 PM, Chris Lattner wrote:
> > > On Jun 15, 2012, at 3:48 PM, Nick Kledzik wrote:
> > >
> > >> In lld we have a Reader class which is a factory that takes
.o
> file(s) and produces an in memory lld::File object(s).  But in doing so,
> there could be I/O errors (file not readable) or file may be malformed.  We
> are also using C++11 in lld, so we use std::unique_ptr for managing object
> ownership.
> > >>
> > >> The Reader class currently has an interface that can be
simplified
> down to:
> > >>
> > >>   virtual error_code readFile(StringRef path,
> std::unique_ptr<lld::File> &result);
> > >>
> > >> But this interface has become awkward to use. There are two
"return"
> values.  This method is a unique_ptr "source" but the use of a
by-refernce
> parameter means you have to start with an uninitialized unique_ptr and the
> readFile() has to std::move() into it.   unique_ptr is much nicer to use as
> a return value.
> > >
> > > Not a c++'11 expert here, but does a returning an std::tuple
work?
> > I talked to Howard about that.  It does work, but it is cumbersome.
>  Either:
> >
> > std::tuple<std::unique_ptr<lld::File>, error_code>  result
> reader.readFile(path);
> >
>
> I suggest an error_or<T> class that can be combined with any result
type,
> not just File. It could be implemented with a fully generic variant class,
> but the interface would likely be better with some specialization to
errors.
>
I also really like the variant design. It has a few important advantages
IMO:

1) No pointers or heap allocations. These make code more bug prone and can
needlessly hurt optimizers. (although the latter is hopefully not relevant
in this specific case)

2) No "error" state in the file object. Either you have a fully
constructed
object or you don't.

3) An explicit API to check and instrument whether an error has occured.
For example, it is harder to "forget" to check the error, and carry on
manipulating the file. The implementation can assert in debug builds to
catch this early, etc.

4) Doesn't require polymorphic types (but they can be supported if needed).
Essentially, you can wrap a *specific* file type, rather than being forced
to use some generic file type.


One interesting possibility is that you can grow the same pattern to
different levels of complexity as needed (and thus keep it as simple as
possible as long as possible):

error_or<T>  -- wraps the existing error_code with a nice interface
linker_error_or<T> -- wraps both an error_code and some linker specific
error handling logic
file_error_or<T> -- provides completely custom error information specific
to file-loading-errors.


I'm not saying which (if any) of these would be appropriate, just pointing
out that this pattern can be followed to greater and lesser degrees of
specificity.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120620/ffc3b999/attachment.html>