Quuxplusone / LLVMBugzillaTest

0 stars 0 forks source link

Attributes order, it is meaningful and clang loses it. #3623

Open Quuxplusone opened 15 years ago

Quuxplusone commented 15 years ago
Bugzilla Link PR3268
Status NEW
Importance P normal
Reported by bolzoni (bolzoni@cs.unipr.it)
Reported on 2008-12-28 11:45:23 -0800
Last modified on 2010-03-12 00:59:13 -0800
Version unspecified
Hardware All All
CC anton@korobeynikov.info, bagnara@cs.unipr.it, daniel@zuster.org, efriedma@quicinc.com, llvm-bugs@lists.llvm.org
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also
It is critical in presence of __asm__ attribute, the __asm__ attribute must be
first.
Instead in this code:
---->
typedef unsigned long size_t;
extern int strerror_r(int __errnum, char *__buf, size_t __buflen)
  __asm__ ("" "__xpg_strerror_r")
  __attribute__ ((__nothrow__))
  __attribute__ ((__nonnull__ (2)));
----<
The asm attribute comes out last.
Quuxplusone commented 15 years ago

I don't follow... what's the visible issue you're seeing?

Quuxplusone commented 15 years ago
The issue is in the clang's AST.
The attributes order in the int strerror() declaration is:
 no_throw
 non_null([2])
 asm_label(__xpg_strerror_r)

instead of
 asm_label(__xpg_strerror_r)
 no_throw
 non_null([2])
Quuxplusone commented 15 years ago

Can someone please comment on this?

If clang has good reasons to reorder the attributes, please let us know. In this case we will write code to reorder the attributes so as to make the sequence legal. But it would be a pity to do such a hack if the right solution is simply to stop clang from reordering them.

Quuxplusone commented 15 years ago

I don't understand how one is semantically different than the other. Why do you care of asm is first or last?

Quuxplusone commented 15 years ago

Because asm is legal only if it is first. Since in my application I have (among other things) to warn the programmer about the fact he/she has got the ordering wrong, the fact that clang reorders the attributes makes this task difficult/impossible without reparsing.

More generally, the attribute mechanism is something relatively fluid (new attributes can and will be added): it is possible there will be other reasons not to shuffle the attributes.

Quuxplusone commented 15 years ago

Ok, so it doesn't break any existing clients.

Quuxplusone commented 15 years ago
Another thing to consider is that, for gcc, asm("label") is *not* an attribute
and clang's choice to represent it as an attribute is questionable.  For
example, attributes can occur in function definitions, but asm("label") is only
valid in function declarations.

Moreover, clang merges attributes for the same function found in different
declarations and in the definition, and subsequent declarations/definition
contain also the attributes present in the preceding ones.  This result in
asm("label") being applied to illegal contexts.
Quuxplusone commented 15 years ago

Are we actually going to fix this?

Quuxplusone commented 15 years ago

We definitely hope so. Has comment #7 been given proper consideration?

Quuxplusone commented 15 years ago

Clang's attributes are just away to attach information to a decl "on the side", without taking space in every declaration. Nothing necessarily implies that what clang models this way has to be an actual attribute. I don't see any problem with modeling asm this way.

I do agree there are merging considerations of attributes that clang loses (or should be more careful about). However, that isn't really what this bug is about.