Some elements of local structure are missing

[sslupsky]

I have come across another issue I have noticed several times. Some elements of a struct are missing in the local window. I've attached a screen shot and a copy of the function I was debugging at the time below. In this example, notice that "thread_base->prio" and "thread_base->sched_locked" do not appear in the local window though other elements of the struct such as "thread_base->thread_state" do appear.

I also attached some output from GDB.

void z_init_thread_base(struct _thread_base *thread_base, int priority,
               u32_t initial_state, unsigned int options)
{
    /* k_q_node is initialized upon first insertion in a list */

    thread_base->user_options = (u8_t)options;
    thread_base->thread_state = (u8_t)initial_state;

    thread_base->prio = priority;

    thread_base->sched_locked = 0U;

#ifdef CONFIG_SMP
    thread_base->is_idle = 0;
#endif

    /* swap_data does not need to be initialized */

    z_init_thread_timeout(thread_base);
}

print thread_base
$1 = (struct _thread_base *) 0x200002d0 <_k_thread_obj_led_process_id>

{"token":927,"outOfBandRecord":[],"resultRecords":{"resultClass":"done","results":[]}}

info symbol thread_base
_k_thread_obj_led_process_id in section bss
{"token":958,"outOfBandRecord":[],"resultRecords":{"resultClass":"done","results":[]}}

info frame 0
Stack frame at 0x200045d8:
 pc = 0x186f8 in z_init_thread_base (/Users/stevenslupsky/Documents/source/zephyrproject/zephyr/kernel/thread.c:817); saved pc = 0x18710
 called by frame at 0x200045e0
 source language c.
 Arglist at 0x200045d8, args: thread_base=thread_base@entry=0x200002d0 <_k_thread_obj_led_process_id>, priority=priority@entry=7, initial_state=initial_state@entry=4, options=options@entry=0
 Locals at 0x200045d8, Previous frame's sp is 0x200045d8
{"token":1051,"outOfBandRecord":[],"resultRecords":{"resultClass":"done","results":[]}}

info locals
No locals.
{"token":865,"outOfBandRecord":[],"resultRecords":{"resultClass":"done","results":[]}}

info args
thread_base = 0x200002d0 <_k_thread_obj_led_process_id>

priority = 7

initial_state = 4

options = 0

{"token":896,"outOfBandRecord":[],"resultRecords":{"resultClass":"done","results":[]}}

[haneefdm]

The fields you are looking for are actually in an unnamed union. While prio may actually look like a direct field, it is actually buried in the unnamed union/struct. Could you please expand the union I see in your screen capture and see if it exists there.

/* can be used for creating 'dummy' threads, e.g. for pending on objects */
struct _thread_base {

    /* this thread's entry in a ready/wait queue */
    union {
        sys_dnode_t qnode_dlist;
        struct rbnode qnode_rb;
    };

    /* wait queue on which the thread is pended (needed only for
     * trees, not dumb lists)
     */
    _wait_q_t *pended_on;

    /* user facing 'thread options'; values defined in include/kernel.h */
    u8_t user_options;

    /* thread state */
    u8_t thread_state;

    /*
     * scheduler lock count and thread priority
     *
     * These two fields control the preemptibility of a thread.
     *
     * When the scheduler is locked, sched_locked is decremented, which
     * means that the scheduler is locked for values from 0xff to 0x01. A
     * thread is coop if its prio is negative, thus 0x80 to 0xff when
     * looked at the value as unsigned.
     *
     * By putting them end-to-end, this means that a thread is
     * non-preemptible if the bundled value is greater than or equal to
     * 0x0080.
     */
    union {
        struct {
#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
            u8_t sched_locked;
            s8_t prio;
#else /* LITTLE and PDP */
            s8_t prio;
            u8_t sched_locked;
#endif
        };
        u16_t preempt;
    };

#ifdef CONFIG_SCHED_DEADLINE
    int prio_deadline;
#endif

    u32_t order_key;

#ifdef CONFIG_SMP
    /* True for the per-CPU idle threads */
    u8_t is_idle;

    /* CPU index on which thread was last run */
    u8_t cpu;

    /* Recursive count of irq_lock() calls */
    u8_t global_lock_count;

#endif

#ifdef CONFIG_SCHED_CPU_MASK
    /* "May run on" bits for each CPU */
    u8_t cpu_mask;
#endif

    /* data returned by APIs */
    void *swap_data;

#ifdef CONFIG_SYS_CLOCK_EXISTS
    /* this thread's entry in a timeout queue */
    struct _timeout timeout;
#endif
};

[sslupsky]

There is only one anonymous union that appears. The first one shown in the screen shot above expands to show the qnode_dlist and qnode_rb members. The second union with the prio and sched_locked members does not appear in the watch window.

[sslupsky]

If I use the GDB CLI and print thread_base->prio, I get the following:

print thread_base->prio
$2 = 0 '\000'

{"token":646,"outOfBandRecord":[],"resultRecords":{"resultClass":"done","results":[]}}

[haneefdm]

Cortex-Debug only displays what gdb/gcc reports. I will try to reproduce this struct when I get back home. It is possible that two anonymous unions (which are identically named) got merged into one by the Cortex-Debug or VSCode. We may have to generate unique names if that is the case.

[haneefdm]

Okay, narrowed it down. Not a bug with Cortex-Debug but with VSCode itself. We send the right info to VSCode but it, in its infinite wisdom decides to re-interpret the results

• All anonymous unions (and probably structs) have the same display name (the name that you see in the GUI) as assigned by gdb
• It appears that all display names have to be unique so VSCode rejects duplicate ones silently.

Note: The display name is supposed to be user-friendly and the actual name could be quite complicated and hard to understand.

Not easy to change VSCode behavior. Two possible fixes/options to please VSCode. More ideas welcome.

1. Make sure all display names are unique and modify GDB results
2. Flatten anonymous structs/unions - Eclipse does this and so do compilers. Compilers have to, the only way to reference items is to think of flattened ones.

Both are painful and ugly but necessary. I want to hear @Marus opinion. From a UX perspective, I like Option 1 because you can distinguish the union members from the enclosing struct members. The way we have it right now looks good other than the problem with duplicates. As an example

struct _mystruct {
    int x ;
    union {
        int u1a;
        int u1b;
    };
    int y;
    union {
        struct {
            int16_t prio;
            int16_t junk;
        };
        int u2a;
        int u2b;
    };
    int z;
};
volatile struct _mystruct glob;

Here is a screenshot from Eclipse. They used Option 2. From above, I can't tell a union member vs others or any kind of nesting. But it represents how you write code. I am torn but my preferences is Option 1 which closely represents your source code.

I request @Marus to chime in and perhaps the community

If there are no objections, I will proceed with implementing Option 1 in a couple of days

[haneefdm]

Oh, for reference this is what you see in Cortex-Debug. Missing parts of the struct

[sslupsky]

At first I interpreted "flattening" as flattening the entire thing but I see from the example that Eclipse flattens only the contents of the struct.

So, to my eye, option 2 actually represents how you would access the struct in code. That is,

glob.x glob.u1a glob.prio

We are talking about flattening only the anonymous unions and structs, correct? Any unions or structs that are not anonymous would not be flattened, correct?

If so, I think option 2 is a reasonable approach. I think it might be advantageous to consider being consistent with the way Eclipse (and others?) are handling it if there is no compelling reason to be different.

[haneefdm]

Yes, only flattening the anonymous stuff if we do it that way. Even if eclipse does it that way, I lost important information about the data-structure which is the struct layout. GCC & GDB try hard to preserve the layout and Eclipse just removed all that information.

[haneefdm]

Here is my initial implementation. Anonymous unions/structs are numbered if there are duplicates within the same scope or else, we respect gdb's wishes for the display name. The trick was to make sure if you right-click (in the Variables Window) and add an item to the Watch window, that should still work. If you right-click on an actual field, you will get the proper reference but if you add an anonymous union/struct to the Watch list, you actually get the parent added. Needs lot more testing.

[haneefdm]

Source code changes to my fork have been pushed. Will do a PR after looking at Issue #226

[haneefdm]

I have an alternate implementation that matches Eclipse, Visual Studio and VS Code for desktop C++ projects. They all flatten classes/structs to remove anonymous unions/structs. I will be removing my previous implementation. I will be merging my changes shortly

struct _mystruct {
    int x ;
    union {
        int u1a;
        int u1b;
    };
    union {
        int u1aa;
        int u1bb;
    };
    int y;
    union {
        struct {
            union {
                int8_t a, b;
            };
            union {
                int16_t prio;
                int16_t junk;
            };
        };
        int u2a;
        int u2b;
    };
    int z;
};
volatile struct _mystruct glob;``

[haneefdm]

Closing issue since I created a PR

[haneefdm]

I take back my final implementation. I talked to several developers and they actually prefer Option#1 where the anonymous unions/structs are not flattened. This is how it was before. What they saw in not flattening as an improvement and a better user experience. The way VStudio and Eclipse do it not good from a programmer perspective I believe. We could advance the UX vs copying an old method. Let others copy us :-)

[haneefdm]

There is a beta release now available. I am closing this issue. See ChangeLog

https://github.com/Marus/cortex-debug/releases/tag/v0.3.5-beta1