linux-sparse.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Christopher Li <sparse@chrisli.org>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Josh Triplett <josh@joshtriplett.org>,
	Sparse Mailing-list <linux-sparse@vger.kernel.org>
Subject: Re: Designated initializers for fields in anonymous structs and unions
Date: Fri, 1 Aug 2014 22:16:59 -0700	[thread overview]
Message-ID: <CANeU7Qn4++T+GsL6pjnEem0SOWhGrvBN7TLC8p8Hjbw2xCmVbw@mail.gmail.com> (raw)
In-Reply-To: <CA+55aFwfN1L6U=6+_a-Ue_Z6tPNfphKkpidtdD06zs+tHLPyJQ@mail.gmail.com>

On Fri, Aug 1, 2014 at 6:16 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Actually, sadly, while that worked for my test-cases (and for Josh's
> case), it turns out to really *not* work in general: the dummy
> EXPR_IDENTIFIER that we create may end up referring to a unnamed
> union, which doesn't *have* an ident associated with it.
>
> This only really triggers when you mix named initializers with then
> relying on the whole "initialize the next one without a name", but
> that does happen. And it fundamentally breaks that whole "use
> find_identifier()" approach.
>
> So convert_ident() really cannot use find_indent(), and really does
> have to use "sym->offset". But that doesn't work for anonymous union
> members, because that offset was the offset within the anonymous
> union, not the parent structure.
>
> However, the fix is "simple". We can just say that anonymous
> union/structure member offsets have to be the offsets within the
> parent union instead, and fix things up. So how about a patch like
> this *instead* of the previous patch..

Sorry I am late for the party. This approach assume the anonymous
structure is only referenced inside the structure. There is not other
external reference. However, that assumption is not true if "-fplan9-extensions"
or "-fms-extensions" was enabled. Sparse current does not support these
two extensions. But it will be a problem if we support them down the road.

According to this gcc document:

https://gcc.gnu.org/onlinedocs/gcc/Unnamed-Fields.html

One of the example is this:
     struct s1 { int a; };
     struct s2 { struct s1; };

You see, "struct s1" does not have a name, however "struct s1" can be
referenced from other symbol. So update the symbol offset in "struct s1"
will be wrong for the other reference of "struct s1".

Actually, the "find_identifier" call inside function "check_designators" already
calculate the correct offset. Only if we can pass that offset to
"convert_ident()".

Another idea is that, instead of using ident in EXPR_IDENTIFIER, we can
do one time find_identifier(). Then convert the EXPR_IDENTIFIER to an index
to the struct. It might take more than one index level for the anonymous
struct or union. BTW, that is very similar to the LLVM  performing the
"Get Element Pointer" instruction. Using the index, we can still reference
the anonymous struct.  This is not a trivial change. However it might be
the right thing to do if we want to support "-fplan9-extensions"

Chris

  reply	other threads:[~2014-08-02  5:17 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-07-31 18:10 Designated initializers for fields in anonymous structs and unions josh
2014-07-31 18:39 ` Linus Torvalds
2014-07-31 18:50   ` Linus Torvalds
2014-07-31 20:55     ` josh
2014-08-02  8:27     ` Christopher Li
2014-08-02 18:09       ` Christopher Li
2014-08-01  2:19 ` Linus Torvalds
2014-08-01  2:41   ` Linus Torvalds
2014-08-02  1:16     ` Linus Torvalds
2014-08-02  5:16       ` Christopher Li [this message]
2014-08-02 18:10         ` Linus Torvalds
2014-08-02 18:31           ` Derek M Jones
2014-08-02 18:40             ` Christopher Li
2014-08-02 19:53           ` Linus Torvalds
2014-08-02 20:09             ` Linus Torvalds
2014-08-06  9:15             ` Christopher Li
2014-08-01  2:41   ` Josh Triplett

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CANeU7Qn4++T+GsL6pjnEem0SOWhGrvBN7TLC8p8Hjbw2xCmVbw@mail.gmail.com \
    --to=sparse@chrisli.org \
    --cc=josh@joshtriplett.org \
    --cc=linux-sparse@vger.kernel.org \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).