[Pdbg] [PATCH v2 8/9] main: Overhaul target selection logic

[Balbir Singh]

Resending with the correct "Cc"

On Thu, May 24, 2018 at 11:35 AM, Balbir Singh <bsingharora at gmail.com> wrote:
> On Wed, May 23, 2018 at 2:31 PM, Amitay Isaacs <amitay at ozlabs.org> wrote:
>> This should now fix the logic for -a working in conjunction with
>> -p/-c/-t to select all threads.
>>
>>  -a            select all threads
>>  -a -p 0       select all threads on processor 0
>>  -p 0 -a       select all threads on processor 0
>>  -p 0 -c 1 -a  select all threads on processor 0, chip 1
>>
>> Signed-off-by: Amitay Isaacs <amitay at ozlabs.org>
>> ---
>>  src/main.c | 89 ++++++++++++++++++++++++++++++++++++++++++------------
>>  1 file changed, 69 insertions(+), 20 deletions(-)
>>
>> diff --git a/src/main.c b/src/main.c
>> index 65bf6c5..71f82fe 100644
>> --- a/src/main.c
>> +++ b/src/main.c
>> @@ -135,7 +135,7 @@ static void print_usage(char *pname)
>>         printf("\t-c, --chip=<0-%d>\n", MAX_CHIPS-1);
>>         printf("\t-t, --thread=<0-%d>\n", MAX_THREADS-1);
>>         printf("\t-a, --all\n");
>> -       printf("\t\tRun command on all possible processors/chips/threads (default)\n");
>> +       printf("\t\tRun command on all possible processors/chips/threads\n");
>>         printf("\t-b, --backend=backend\n");
>>         printf("\t\tfsi:\tAn experimental backend that uses\n");
>>         printf("\t\t\tbit-banging to access the host processor\n");
>> @@ -170,7 +170,9 @@ static bool parse_options(int argc, char *argv[])
>>  {
>>         int c;
>>         bool opt_error = true;
>> -       static int current_processor = INT_MAX, current_chip = INT_MAX, current_thread = INT_MAX;
>> +       static int start_p = INT_MAX, start_c = INT_MAX, start_t = INT_MAX;
>> +       static int end_p = INT_MAX, end_c = INT_MAX, end_t = INT_MAX;
>
> This is a bit confusing, why is start_p INT_MAX, effectively we need a
> range, why does the range
> start at INT_MAX and not 0, we need to reset it to 0 down below?
>
>> +       static int cur_p, cur_c, cur_t;
>>         struct option long_opts[] = {
>>                 {"all",                 no_argument,            NULL,   'a'},
>>                 {"backend",             required_argument,      NULL,   'b'},
>> @@ -195,49 +197,53 @@ static bool parse_options(int argc, char *argv[])
>>                 switch(c) {
>>                 case 'a':
>>                         opt_error = false;
>> -                       for (current_processor = 0; current_processor < MAX_PROCESSORS; current_processor++) {
>> -                               processorsel[current_processor] = &chipsel[current_processor][0];
>> -                               for (current_chip = 0; current_chip < MAX_CHIPS; current_chip++) {
>> -                                       chipsel[current_processor][current_chip] = &threadsel[current_processor][current_chip][0];
>> -                                       for (current_thread = 0; current_thread < MAX_THREADS; current_thread++)
>> -                                               threadsel[current_processor][current_chip][current_thread] = 1;
>> -                               }
>> +                       if (start_p == INT_MAX) {
>> +                               start_p = 0;
>> +                               end_p = MAX_PROCESSORS-1;
>> +                       }
>> +                       if (start_c == INT_MAX) {
>> +                               start_c = 0;
>> +                               end_c = MAX_CHIPS-1;
>> +                       }
>> +                       if (start_t == INT_MAX) {
>> +                               start_t = 0;
>> +                               end_t = MAX_THREADS-1;
>
> Why not make these the starting default values above?
>
>>                         }
>>                         break;
>>
>>                 case 'p':
>>                         errno = 0;
>> -                       current_processor = strtoul(optarg, &endptr, 0);
>> +                       cur_p = strtoul(optarg, &endptr, 0);
>>                         opt_error = (errno || *endptr != '\0');
>>                         if (!opt_error) {
>> -                               if (current_processor >= MAX_PROCESSORS)
>> +                               if (cur_p >= MAX_PROCESSORS)
>>                                         opt_error = true;
>>                                 else
>> -                                       processorsel[current_processor] = &chipsel[current_processor][0];
>> +                                       start_p = end_p = cur_p;
>>                         }
>>                         break;
>>
>>                 case 'c':
>>                         errno = 0;
>> -                       current_chip = strtoul(optarg, &endptr, 0);
>> +                       cur_c = strtoul(optarg, &endptr, 0);
>>                         opt_error = (errno || *endptr != '\0');
>>                         if (!opt_error) {
>> -                               if (current_chip >= MAX_CHIPS)
>> +                               if (cur_c >= MAX_CHIPS)
>>                                         opt_error = true;
>>                                 else
>> -                                       chipsel[current_processor][current_chip] = &threadsel[current_processor][current_chip][0];
>> +                                       start_c = end_c = cur_c;
>>                         }
>>                         break;
>>
>>                 case 't':
>>                         errno = 0;
>> -                       current_thread = strtoul(optarg, &endptr, 0);
>> +                       cur_t = strtoul(optarg, &endptr, 0);
>>                         opt_error = (errno || *endptr != '\0');
>>                         if (!opt_error) {
>> -                               if (current_thread >= MAX_THREADS)
>> +                               if (cur_t >= MAX_THREADS)
>>                                         opt_error = true;
>>                                 else
>> -                                       threadsel[current_processor][current_chip][current_thread] = 1;
>> +                                       start_t = end_t = cur_t;
>>                         }
>>                         break;
>>
>> @@ -292,10 +298,53 @@ static bool parse_options(int argc, char *argv[])
>>                 }
>>         } while (c != EOF && !opt_error);
>>
>> -       if (opt_error)
>> +       if (opt_error) {
>>                 print_usage(argv[0]);
>> +               return opt_error;
>> +       }
>> +
>> +       if (start_c != INT_MAX && start_p == INT_MAX) {
>> +               printf("No processor(s) selected\n");
>> +               printf("Use -p to select processor(s)\n");
>
> These should all go to stderr? I think this can be simplified and needs to
> be revisited. I think this patch also subtly breaks other things, in
> the past we could do
>
> pdbg -c0 -t 0 -t 1, now thats broken, IIUC
>
> Balbir