aaron.ballman added a comment.
In https://reviews.llvm.org/D44093#1063658, @paulsemel wrote:
> Sorry about it, I also have the warning on my machine, but not the error you
> get...
> Those test are actually working on my different linux machines, that's
> really weird.
> This one is actually
paulsemel updated this revision to Diff 141920.
paulsemel added a comment.
Added triple option to CodeGen test so that it matches with the correct
architecture
Repository:
rC Clang
https://reviews.llvm.org/D44093
Files:
include/clang/Basic/Builtins.def
lib/CodeGen/CGBuiltin.cpp
aaron.ballman closed this revision.
aaron.ballman added a comment.
I see we all found the solution at the same time. Yay teamwork! :-D
I've committed (with the test fixed up) in r329762.
Repository:
rC Clang
https://reviews.llvm.org/D44093
___
paulsemel added a comment.
Thanks a lot for your help, updating the patch !
Repository:
rC Clang
https://reviews.llvm.org/D44093
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
arichardson added a comment.
In https://reviews.llvm.org/D44093#1063679, @paulsemel wrote:
> Ok, I found the problem. In fact the size of `long` is 4 bytes on your
> machine, but 8 bytes on mine.
> This makes this `// CHECK: [[LOAD1:%[0-9]+]] = load i64, i64* [[RES1]],`
> fail.
> Do you know
arichardson added a comment.
In https://reviews.llvm.org/D44093#1063679, @paulsemel wrote:
> Ok, I found the problem. In fact the size of `long` is 4 bytes on your
> machine, but 8 bytes on mine.
> This makes this `// CHECK: [[LOAD1:%[0-9]+]] = load i64, i64* [[RES1]],`
> fail.
> Do you know
lebedev.ri added inline comments.
Comment at: test/CodeGen/dump-struct-builtin.c:1
+// RUN: %clang_cc1 -emit-llvm %s -o - | FileCheck %s
+
This should be
```
// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -emit-llvm %s -o - | FileCheck %s
```
or something
paulsemel added a comment.
Ok, I found the problem. In fact the size of `long` is 4 bytes on your machine,
but 8 bytes on mine.
This makes this `// CHECK: [[LOAD1:%[0-9]+]] = load i64, i64* [[RES1]],` fail.
Do you know a smart way to do it without dealing with type sizes ?
Repository:
rC
paulsemel updated this revision to Diff 141913.
paulsemel added a comment.
Fixed printf warning generated in tests/CodeGen.
Repository:
rC Clang
https://reviews.llvm.org/D44093
Files:
include/clang/Basic/Builtins.def
lib/CodeGen/CGBuiltin.cpp
lib/Sema/SemaChecking.cpp
paulsemel added a comment.
Sorry about it, I also have the warning on my machine, but not the error you
get...
Those test are actually working on my different linux machines, that's really
weird.
This one is actually really weird, because I could find manually the missing
pattern in your
aaron.ballman added a comment.
Thank you for rebasing, but the patch does not pass tests locally for me. I get
the following results (testing on Windows x64 with MSVC 2017 in a Debug build):
63> TEST 'Clang :: CodeGen/dump-struct-builtin.c' FAILED
paulsemel updated this revision to Diff 141890.
paulsemel added a comment.
Patch rebased.
Minor fix for single quotes escaping.
Repository:
rC Clang
https://reviews.llvm.org/D44093
Files:
include/clang/Basic/Builtins.def
lib/CodeGen/CGBuiltin.cpp
lib/Sema/SemaChecking.cpp
aaron.ballman added a comment.
Can you rebase the patch? The patch did not apply cleanly for me against trunk
(the contents of the check function are appearing in
`CheckARMBuiltinExclusiveCall()`).
(Also, one tiny nit about escaped characters that can be fixed at the same
time.)
paulsemel added a comment.
In https://reviews.llvm.org/D44093#1063348, @aaron.ballman wrote:
> In https://reviews.llvm.org/D44093#1063340, @paulsemel wrote:
>
> > I don't really know what's the procedure right now.. What should I do once
> > you both accepted the patch ? :)
>
>
> You're good to
aaron.ballman added a comment.
In https://reviews.llvm.org/D44093#1063340, @paulsemel wrote:
> I don't really know what's the procedure right now.. What should I do once
> you both accepted the patch ? :)
You're good to commit the patch and address the concerns raised by @arichardson
in a
paulsemel added a comment.
I don't really know what's the procedure right now.. What should I do once you
both accepted the patch ? :)
Repository:
rC Clang
https://reviews.llvm.org/D44093
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
arichardson accepted this revision.
arichardson added a comment.
> So, for the moment, we are only handling basic types. That said, for the enum
> in C, we will print according to the type of the enum.
> In the future versions, I really want to be able to print the name of the
> enum so that
paulsemel added a comment.
In https://reviews.llvm.org/D44093#1063122, @arichardson wrote:
> I'm also often restricted to using printf for debugging so this looks really
> useful!
>
> However, before committing this I feel like the test should also verify that
> the format strings that are
arichardson added a comment.
I'm also often restricted to using printf for debugging so this looks really
useful!
However, before committing this I feel like the test should also verify that
the format strings that are generated are sensible.
Also what should happens when you have enum
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM with some minor commenting nits.
Comment at: lib/CodeGen/CGBuiltin.cpp:992
+
+// We check whether we are in a recursive type
+if
paulsemel updated this revision to Diff 141762.
paulsemel added a comment.
Added a good test for the `int (*)()` function prototype, as we decided to
accept it as a valid function for the dumper
Repository:
rC Clang
https://reviews.llvm.org/D44093
Files:
include/clang/Basic/Builtins.def
paulsemel added inline comments.
Comment at: test/Sema/builtin-dump-struct.c:8
+ void *b;
+ int (*goodfunc)(const char *, ...);
+ int (*badfunc1)(const char *);
aaron.ballman wrote:
> paulsemel wrote:
> > aaron.ballman wrote:
> > > paulsemel wrote:
> > > >
aaron.ballman added inline comments.
Comment at: test/Sema/builtin-dump-struct.c:8
+ void *b;
+ int (*goodfunc)(const char *, ...);
+ int (*badfunc1)(const char *);
paulsemel wrote:
> aaron.ballman wrote:
> > paulsemel wrote:
> > > aaron.ballman wrote:
> > >
paulsemel updated this revision to Diff 141745.
paulsemel marked an inline comment as done.
paulsemel added a comment.
Updated the amperstamp in the Sema test to be consistent with the remaining
part of it.
Repository:
rC Clang
https://reviews.llvm.org/D44093
Files:
paulsemel marked an inline comment as done.
paulsemel added a comment.
No problem, thanks for getting back on this ! I was busy because of my midterms
anyway :)
Comment at: test/Sema/builtin-dump-struct.c:8
+ void *b;
+ int (*goodfunc)(const char *, ...);
+ int
aaron.ballman added a comment.
Sorry for the delayed review; I'm back from vacation now and am picking things
up again.
Comment at: test/Sema/builtin-dump-struct.c:8
+ void *b;
+ int (*goodfunc)(const char *, ...);
+ int (*badfunc1)(const char *);
paulsemel updated this revision to Diff 140304.
paulsemel marked 3 inline comments as done.
paulsemel added a comment.
Added Aaron suggestions.
Fixed a bug when having nested anonymous unions and structures.
Repository:
rC Clang
https://reviews.llvm.org/D44093
Files:
paulsemel marked 6 inline comments as done.
paulsemel added inline comments.
Comment at: test/Sema/builtin-dump-struct.c:8
+ void *b;
+ int (*goodfunc)(const char *, ...);
+ int (*badfunc1)(const char *);
aaron.ballman wrote:
> Can you also add a test for:
aaron.ballman added inline comments.
Comment at: lib/Sema/SemaChecking.cpp:1122
+!PtrArgType->getPointeeType()->isRecordType()) {
+ this->Diag(PtrArg->getLocStart(),
+ diag::err_typecheck_convert_incompatible)
Drop all instances of
paulsemel updated this revision to Diff 138832.
paulsemel added a comment.
Applied Aaron's suggestions
Added Sema tests for the typechecking
Repository:
rC Clang
https://reviews.llvm.org/D44093
Files:
include/clang/Basic/Builtins.def
lib/CodeGen/CGBuiltin.cpp
lib/Sema/SemaChecking.cpp
aaron.ballman added inline comments.
Comment at: lib/CodeGen/CGBuiltin.cpp:935
+static llvm::Value *dumpRecord(CodeGenFunction , QualType RType,
+ Value*& RecordPtr, CharUnits Align,
+ Value *Func, int Lvl)
paulsemel updated this revision to Diff 138687.
paulsemel added a comment.
Added some tests (unit tests for almost every types) and some other tests with
tricky cases.
More tests are coming soon.
Repository:
rC Clang
https://reviews.llvm.org/D44093
Files:
paulsemel added a comment.
Hi,
In https://reviews.llvm.org/D44093#1034610, @lebedev.ri wrote:
> BTW, as far as i can tell this still has zero test coverage (no new tests are
> being added).
> I'd expect to see the tests for the actual output
>
> - one struct per each type it is able to print
lebedev.ri added a comment.
BTW, as far as i can tell this still has zero test coverage (no new tests are
being added).
I'd expect to see the tests for the actual output
- one struct per each type it is able to print
- probably some tests showing error handling,
and possibly the availability
paulsemel updated this revision to Diff 137998.
paulsemel added a comment.
Applied Francis' suggestions
Repository:
rC Clang
https://reviews.llvm.org/D44093
Files:
include/clang/Basic/Builtins.def
lib/CodeGen/CGBuiltin.cpp
lib/Sema/SemaChecking.cpp
Index: lib/Sema/SemaChecking.cpp
thegameg added a comment.
Thanks for working on this! Few remarks in the comments.
Comment at: lib/CodeGen/CGBuiltin.cpp:934
+static Value *dumpRecord(CodeGenFunction , QualType RType,
+ Value*& RecordPtr, CharUnits Align,
paulsemel updated this revision to Diff 137775.
paulsemel added a comment.
Added recursive type pretty-printing as suggested by Aaron.
Repository:
rC Clang
https://reviews.llvm.org/D44093
Files:
include/clang/Basic/Builtins.def
lib/CodeGen/CGBuiltin.cpp
lib/Sema/SemaChecking.cpp
paulsemel added inline comments.
Comment at: lib/CodeGen/CGBuiltin.cpp:1252
+ Types[getContext().getPointerType(getContext().CharTy)] = "%s";
+ GENERATE_TYPE_QUALIFIERS_NO_RESTRICT(getContext().CharTy, "%s")
+}
aaron.ballman wrote:
> paulsemel
aaron.ballman added inline comments.
Comment at: lib/CodeGen/CGBuiltin.cpp:1231
+ Types[getContext().VoidPtrTy] = "%p";
+ Types[getContext().FloatTy] = "%f";
+ Types[getContext().DoubleTy] = "%f";
paulsemel wrote:
> aaron.ballman wrote:
> >
paulsemel updated this revision to Diff 137578.
paulsemel marked 3 inline comments as done.
paulsemel added a comment.
Applied Aaron suggestions
Repository:
rC Clang
https://reviews.llvm.org/D44093
Files:
include/clang/Basic/Builtins.def
lib/CodeGen/CGBuiltin.cpp
paulsemel marked 12 inline comments as done.
paulsemel added inline comments.
Comment at: lib/CodeGen/CGBuiltin.cpp:1231
+ Types[getContext().VoidPtrTy] = "%p";
+ Types[getContext().FloatTy] = "%f";
+ Types[getContext().DoubleTy] = "%f";
aaron.ballman added inline comments.
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:5022-5023
+def err_dump_struct_invalid_argument_type : Error<
+ "invalid argument of type %0; expected %1">;
+
Can you look to see if we have an existing diagnostic
paulsemel updated this revision to Diff 137371.
paulsemel added a comment.
Updated with more context.
Repository:
rC Clang
https://reviews.llvm.org/D44093
Files:
include/clang/Basic/Builtins.def
include/clang/Basic/DiagnosticSemaKinds.td
lib/CodeGen/CGBuiltin.cpp
lebedev.ri added a comment.
Please always upload patches with full context (-U99)
Repository:
rC Clang
https://reviews.llvm.org/D44093
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
paulsemel updated this revision to Diff 137369.
paulsemel added a comment.
Applied Aaron suggestion changes.
Added parameters checking in Sema.
Repository:
rC Clang
https://reviews.llvm.org/D44093
Files:
include/clang/Basic/Builtins.def
include/clang/Basic/DiagnosticSemaKinds.td
paulsemel marked 2 inline comments as done.
paulsemel added inline comments.
Comment at: lib/CodeGen/CGBuiltin.cpp:1206
+QualType Arg0Type = Arg0->getType()->getPointeeType();
+const RecordType *RT = Arg0Type->getAs();
+
aaron.ballman wrote:
> You can
aaron.ballman added inline comments.
Comment at: lib/CodeGen/CGBuiltin.cpp:1208
+
+assert(RT && "The first argument must be a record type");
+
paulsemel wrote:
> aaron.ballman wrote:
> > I don't see anything enforcing this constraint, so users are likely to
paulsemel updated this revision to Diff 137141.
paulsemel added a comment.
Applied Aaron change suggestions
Repository:
rC Clang
https://reviews.llvm.org/D44093
Files:
include/clang/Basic/Builtins.def
lib/CodeGen/CGBuiltin.cpp
Index: lib/CodeGen/CGBuiltin.cpp
paulsemel marked 4 inline comments as done.
paulsemel added inline comments.
Comment at: lib/CodeGen/CGBuiltin.cpp:1208
+
+assert(RT && "The first argument must be a record type");
+
aaron.ballman wrote:
> I don't see anything enforcing this constraint, so
aaron.ballman added reviewers: echristo, rsmith.
aaron.ballman added a comment.
Adding some reviewers.
One thing that's missing from this patch are test cases that demonstrate both
the semantic checking (builtin accepts proper args, rejects improper ones, etc)
and output.
paulsemel created this revision.
paulsemel added a reviewer: aaron.ballman.
The purpose of this new builtin is to be able to pretty print any structure at
runtime.
This might be really useful when debugging is not an option or is fastidious
(like for kernel development).
Repository:
rC
51 matches
Mail list logo
