[PATCH] D45383: Strip reference from a va_list object in C when merging parameter types.

2018-04-06 Thread Erich Keane via Phabricator via cfe-commits
erichkeane created this revision. erichkeane added reviewers: efriedma, eli.friedman, compnerd, rsmith. erichkeane added a project: clang. As reported here: https://bugs.llvm.org/show_bug.cgi?id=37033 Any usage of a builtin function that uses a va_list by reference will cause an assertion when red

[PATCH] D45383: Strip reference from a va_list object in C when merging parameter types.

2018-04-06 Thread Eli Friedman via Phabricator via cfe-commits
efriedma added a comment. Rather than adding weird hacks to merging, can we just reject any attempt to redeclare a builtin? Repository: rC Clang https://reviews.llvm.org/D45383 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists

[PATCH] D45383: Strip reference from a va_list object in C when merging parameter types.

2018-04-06 Thread Erich Keane via Phabricator via cfe-commits
erichkeane added a comment. In https://reviews.llvm.org/D45383#1060104, @efriedma wrote: > Rather than adding weird hacks to merging, can we just reject any attempt to > redeclare a builtin? I don't believe we can. I think things often re-declare them, particularly when it comes to va_end.

[PATCH] D45383: Strip reference from a va_list object in C when merging parameter types.

2018-04-06 Thread Erich Keane via Phabricator via cfe-commits
erichkeane added a comment. One big issue with just disallowing redecls of builtins is we have is that a number of these are actually expected to be in the system headers/libraries (such as vprintf). A ton of the math.h functions are also builtins. Repository: rC Clang https://reviews.llvm

[PATCH] D45383: Strip reference from a va_list object in C when merging parameter types.

2018-04-06 Thread Eli Friedman via Phabricator via cfe-commits
efriedma added a comment. By "builtins" I meant specifically the stuff that that user code isn't allowed to declare, like __builtin_*, since they often have weird/incomplete signatures. Repository: rC Clang https://reviews.llvm.org/D45383 ___ cf

[PATCH] D45383: Strip reference from a va_list object in C when merging parameter types.

2018-04-06 Thread Erich Keane via Phabricator via cfe-commits
erichkeane added a comment. In https://reviews.llvm.org/D45383#1060275, @efriedma wrote: > By "builtins" I meant specifically the stuff that that user code isn't > allowed to declare, like __builtin_*, since they often have weird/incomplete > signatures. Unfortunately, the problem shows in vp

[PATCH] D45383: Strip reference from a va_list object in C when merging parameter types.

2018-04-06 Thread Eli Friedman via Phabricator via cfe-commits
efriedma added a comment. How could this patch possibly affect vprintf? Repository: rC Clang https://reviews.llvm.org/D45383 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45383: Strip reference from a va_list object in C when merging parameter types.

2018-04-09 Thread Erich Keane via Phabricator via cfe-commits
erichkeane added a comment. In https://reviews.llvm.org/D45383#1060354, @efriedma wrote: > How could this patch possibly affect vprintf? vprintf is defined in the builtins.def file, and other than our messages considering it a 'library function', it is otherwise identical. Repository: rC C

[PATCH] D45383: Strip reference from a va_list object in C when merging parameter types.

2018-04-09 Thread Eli Friedman via Phabricator via cfe-commits
efriedma added a comment. Identical to what? `__builtin_va_start` and `__builtin_va_end` specifically are weird because they're builtins which have a signature which can't be expressed in C. vprintf doesn't have that problem. Builtins.def makes the relevant distinction already: a "BUILTIN" is

[PATCH] D45383: Strip reference from a va_list object in C when merging parameter types.

2018-04-09 Thread Erich Keane via Phabricator via cfe-commits
erichkeane added a comment. In https://reviews.llvm.org/D45383#1061780, @efriedma wrote: > Identical to what? `__builtin_va_start` and `__builtin_va_end` specifically > are weird because they're builtins which have a signature which can't be > expressed in C. vprintf doesn't have that problem

[PATCH] D45383: Strip reference from a va_list object in C when merging parameter types.

2018-04-09 Thread Eli Friedman via Phabricator via cfe-commits
efriedma added a comment. > vprintf is handled using the exact same code-paths. SO, it'll have its 2nd > parameter created with type 'char*&' vprintf is defined in the C standard as "int vprintf(const char *format, va_list arg);"; on Windows, that's equivalent to "int vprintf(const char *forma

[PATCH] D45383: Strip reference from a va_list object in C when merging parameter types.

2018-04-09 Thread Erich Keane via Phabricator via cfe-commits
erichkeane added a comment. In https://reviews.llvm.org/D45383#1061797, @efriedma wrote: > > vprintf is handled using the exact same code-paths. SO, it'll have its 2nd > > parameter created with type 'char*&' > > vprintf is defined in the C standard as "int vprintf(const char *format, > va_list

[PATCH] D45383: Strip reference from a va_list object in C when merging parameter types.

2018-04-09 Thread Eli Friedman via Phabricator via cfe-commits
efriedma added a comment. > I don't see redeclaring the latter 3 as being disallowed, It is in fact disallowed by the standard, in the section describing va_start/va_arg/va_end/va_copy: "If a macro definition is suppressed in order to access an actual function, or a program defines an external

[PATCH] D45383: Strip reference from a va_list object in C when merging parameter types.

2018-04-09 Thread Erich Keane via Phabricator via cfe-commits
erichkeane added a comment. In https://reviews.llvm.org/D45383#1062014, @efriedma wrote: > > I don't see redeclaring the latter 3 as being disallowed, > > It is in fact disallowed by the standard, in the section describing > va_start/va_arg/va_end/va_copy: "If a macro definition is suppressed in

[PATCH] D45383: Strip reference from a va_list object in C when merging parameter types.

2018-04-09 Thread Eli Friedman via Phabricator via cfe-commits
efriedma added a comment. > I believe it is permissible to implement va_copy and va_end as a function, > isn't it? I guess you could, in theory, but LLVM doesn't support any targets which do that. Repository: rC Clang https://reviews.llvm.org/D45383 _

[PATCH] D45383: Strip reference from a va_list object in C when merging parameter types.

2018-04-09 Thread Erich Keane via Phabricator via cfe-commits
erichkeane added a comment. In https://reviews.llvm.org/D45383#1062078, @efriedma wrote: > > I believe it is permissible to implement va_copy and va_end as a function, > > isn't it? > > I guess you could, in theory, but LLVM doesn't support any targets which do > that. Ok, fair enough. So is

[PATCH] D45383: Strip reference from a va_list object in C when merging parameter types.

2018-04-09 Thread Eli Friedman via Phabricator via cfe-commits
efriedma added a comment. Basically, yes, although I was thinking you could be a bit more aggressive. At least, anything marked "t" for custom typechecking is probably not something which should be redeclared. Repository: rC Clang https://reviews.llvm.org/D45383

[PATCH] D45383: Strip reference from a va_list object in C when merging parameter types.

2018-04-09 Thread Erich Keane via Phabricator via cfe-commits
erichkeane added a comment. In https://reviews.llvm.org/D45383#1062099, @efriedma wrote: > Basically, yes, although I was thinking you could be a bit more aggressive. > At least, anything marked "t" for custom typechecking is probably not > something which should be redeclared. Ok, thank you