This is an archive of the discontinued LLVM Phabricator instance.

[libc] Implement getopt
ClosedPublic

Authored by abrachet on Sep 8 2022, 6:07 AM.

Diff Detail

Event Timeline

abrachet created this revision.Sep 8 2022, 6:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 8 2022, 6:07 AM
abrachet requested review of this revision.Sep 8 2022, 6:07 AM

I wanted to get this out fairly early because there are quite a few things that I wanted to get your input on hdr-gen and policy questions etc

libc/spec/posix.td
586

This is a bit of a sad situation. Do you have any thoughts on this, @sivachandra

592

I've just remembered I need to add ObjectSpec's for the global objects. I'll do that in the next round

libc/src/unistd/getopt.cpp
108

How should we be pulling in stdio bits? It seems like we've been trying to make an effort to call other __llvm_libc:: entrypoints from each other. Though fprintf is sufficiently big that I think we would have to just do this. Alternatively we could just use fprintf and that could be either the system fprintf or happen to be llvm-libc's if thats linked in. I'm not sure what our policy is here.

libc/src/unistd/getopt.h
17

Currently the only prior art for extern symbols like this is std{in,out,err}. Though I have some reservations on how those are implemented/tested. Functions being in the __llvm_libc namespace for testing is really useful and I think we should be able to do the same for global objects with aliases just like for functions to ensure hermeticity. I wanted to get your opinion on if you had tried this before.

sivachandra added inline comments.Sep 8 2022, 10:44 PM
libc/spec/posix.td
586

The trick would be to declare an internal type like:

typedef char *const __getopt_argv_type[];

In the tablegen files, define a NamedType for it and add a type header like this: https://github.com/llvm/llvm-project/blob/main/libc/include/llvm-libc-types/__atexithandler_t.h

592

SGTM

libc/src/unistd/getopt.cpp
108

Can we extend GetoptState [1] with an errstream field which:

  1. Defaults to __llvm_libc::stderr.
  2. Tests can override with their own custom File specialization, say for example a StrFile, which writes to a string buffer.

[1] - May be we should give GetoptState a better name.

libc/src/unistd/getopt.h
17

I think the examples of stdin and stdout are inappropriate for future global state variables. I want to suggest the following:

  1. Do not declare the getopt state variables here. Instead, provide a method by name set_getopt_state which is declared as:
void set_getopt_state(char **optarg, int *optind, int *optopt, int optpos, int opterr);
  1. In the .cpp file do this:
namespace impl {

extern "C" char *optarg;
extern "C" int optind = 1;
extern "C" int optopt;
extern "C" int opterr;
extern "C" int optpos = 0;

}

struct GetoptState {
  char **optarg;
  int *optind;
  int *optopt;
  unsigned *optpos;

  int opterr;
};
// The default state
static GetoptState state{&impl::optarg, &impl::optind, &impl::optopt, &impl::optpos, impl::opterr};

void set_getopt_state(char **optarg, int *optind, int *optopt, int *optpos, int opterr) {
  state.optarg = optarg;
  state.optind = optind;
  state.optopt = optopt;
  state.optpos = optpos;
  state.opterr = opterr;
}

The high level goal is to let tests specify where the state of getopt is to be stored by calling set_getopt_state. We can choose to exclude that function under LLVM_LIBC_PUBLIC_PACKAGING.

abrachet updated this revision to Diff 461574.Sep 20 2022, 8:44 AM
abrachet marked 2 inline comments as done.
abrachet edited the summary of this revision. (Show Details)
abrachet marked an inline comment as done.Sep 20 2022, 8:47 AM
abrachet added inline comments.
libc/spec/posix.td
586

Ah that's right. I forgot about that. Thanks

libc/src/unistd/getopt.cpp
108

I've named it GetoptContext, which might be a better name.

I've used just stderr instead of __llvm_libc::stderr and same with fprintf. We were hoping to use this on Windows, actually, and pulling in llvm-libc's stdio isn't ready yet in that context. Likewise we are likely to pull this into Fuchsia's libc before stdio too. It would be helpful if we could do with stdio here like we do with malloc elsewhere. Is that okay?

Still a few comments about the high-level structuring. Once these are sorted out, I will do a pass over the parsing logic.

libc/src/unistd/getopt.cpp
108

This problem is likely not unique to getopt. WDYT about a build flag LIBC_USE_EXTERNAL_STDIO which gets converted to compile option -DUSE_EXTERNAL_STDIO. In the source code, when USE_EXTERNAL_STDIO is defined, we use public names to refer to symbols. By default, we use __llvm_libc::<name>.

libc/src/unistd/getopt.h
17

Do we still need the extern declarations?

libc/test/src/unistd/getopt_test.cpp
19

This can go into the implementation header. Any reason not to put it there?

60

This is going to make a lot of things a little messy - I am fine with your code here, but I am just thinking about how this is going to work in the FULL_BUILD mode.

May be tests can also look at the LIBC_USE_EXTERNAL_STDIO flag and conditionally add the appropriate DEPENDS. In the source code:

#ifdef USE_EXTERNAL_STDIO
define FOPENCOOKIE fopencookie
...
#else
#include "src/stdio/fopencookie.h"
define FOPENCOOKIE __llvm_libc::fopencookie
...
#endif
68

reset_errstream?

69

get_error_msg?

abrachet updated this revision to Diff 468730.Oct 18 2022, 4:02 PM
abrachet marked 4 inline comments as done.
abrachet marked 3 inline comments as done.Oct 18 2022, 4:05 PM
abrachet added inline comments.
libc/src/unistd/getopt.cpp
108

For now, I'll just use __llvm_libc::* and deal with using external stdio later.

libc/src/unistd/getopt.h
17

Nope, thanks.

libc/test/src/unistd/getopt_test.cpp
60

Per other comment, I've just gone and used __llvm_libc::fopencookie

sivachandra accepted this revision.Oct 19 2022, 12:58 AM

LGTM with nits.

libc/src/unistd/getopt.cpp
18

Add a comment here saying that this implementation currently does not support GNU extensions.

41

report_error

149

Subjective comment - ISTM that I could have followed this logic readily if it was arranged this way:

++ctx.optind;
if (ctx.optind >= argc ...) {
}
ctx.optarg.get() = argv[ctx.optind];
libc/test/src/unistd/getopt_test.cpp
116

We actually follow this style: int('?') Better may be, like you have added the _c suffix, add _i suffix.

This revision is now accepted and ready to land.Oct 19 2022, 12:58 AM
abrachet updated this revision to Diff 469274.Oct 20 2022, 10:22 AM
abrachet marked 5 inline comments as done.

Going to let the premerge run before pushing, because it wasn't working without full build before

abrachet marked an inline comment as done.Oct 20 2022, 10:23 AM
abrachet added inline comments.
libc/src/unistd/getopt.cpp
149

I've added a comment to explain why I prefer to keep it as is, because optind should stay at the previous value upon return

abrachet updated this revision to Diff 470966.Oct 26 2022, 4:19 PM
abrachet marked an inline comment as done.

Rebase to let presubmit run

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 26 2022, 11:24 PM

Looks like libc-api-test is failing. I'll look at it tomorrow