Diff Detail
Event Timeline
I'll read this patch as much as possible and try to get the whole picture. But because much of the code is already reviewed by llgo developers, I don't assume that the same level of code review as other patches is needed. I'll mainly look for obvious red signals (if exists).
So, I think the important question here is -- should this go (ba-dum!) in a
separate subproject much the way clang, lld, and lldb do? I think the
answer is "yes".
CC-ing Chris for comment on this.
2014-11-19 15:51 GMT-06:00 Peter Collingbourne <peter@pcc.me.uk>:
Hi ruiu, chandlerc,
Files:
cmake/modules/AddLLVM.cmake tools/CMakeLists.txt tools/llgo/.gitignore tools/llgo/CMakeLists.txt tools/llgo/LICENSE.TXT tools/llgo/README.TXT tools/llgo/build/context.go tools/llgo/cmd/cc-wrapper/main.go tools/llgo/cmd/gllgo/gllgo.go tools/llgo/debug/debug.go tools/llgo/irgen/annotations.go tools/llgo/irgen/attribute.go tools/llgo/irgen/builtins.go tools/llgo/irgen/cabi.go tools/llgo/irgen/call.go tools/llgo/irgen/channels.go tools/llgo/irgen/closures.go tools/llgo/irgen/compiler.go tools/llgo/irgen/errors.go tools/llgo/irgen/indirect.go tools/llgo/irgen/interfaces.go tools/llgo/irgen/maps.go tools/llgo/irgen/parser.go tools/llgo/irgen/predicates.go tools/llgo/irgen/println.go tools/llgo/irgen/runtime.go tools/llgo/irgen/slice.go tools/llgo/irgen/ssa.go tools/llgo/irgen/strings.go tools/llgo/irgen/targets.go tools/llgo/irgen/typemap.go tools/llgo/irgen/types.go tools/llgo/irgen/utils.go tools/llgo/irgen/value.go tools/llgo/irgen/version.go tools/llgo/libgo-noext.diff tools/llgo/llgo-go.sh tools/llgo/ssaopt/esc.go tools/llgo/test/CMakeLists.txt tools/llgo/test/debuginfo/emptyname.go tools/llgo/test/execution/Inputs/init2.go tools/llgo/test/execution/arrays/compare.go tools/llgo/test/execution/arrays/index.go tools/llgo/test/execution/arrays/range.go tools/llgo/test/execution/arrays/slice.go tools/llgo/test/execution/assignment/arrays.go tools/llgo/test/execution/assignment/binop.go tools/llgo/test/execution/assignment/dereferencing.go tools/llgo/test/execution/assignment/multi.go tools/llgo/test/execution/assignment/namedresult.go tools/llgo/test/execution/branching/goto.go tools/llgo/test/execution/branching/labeled.go tools/llgo/test/execution/chan/buffered.go tools/llgo/test/execution/chan/range.go tools/llgo/test/execution/chan/select.go tools/llgo/test/execution/chan/self.go tools/llgo/test/execution/circulartype.go tools/llgo/test/execution/closures/basic.go tools/llgo/test/execution/closures/issue176.go tools/llgo/test/execution/complex.go tools/llgo/test/execution/const.go tools/llgo/test/execution/conversions/complex.go tools/llgo/test/execution/conversions/float.go tools/llgo/test/execution/conversions/int.go tools/llgo/test/execution/conversions/sameunderlying.go tools/llgo/test/execution/defer.go tools/llgo/test/execution/errors/recover.go tools/llgo/test/execution/for/branch.go tools/llgo/test/execution/fun.go tools/llgo/test/execution/functions/compare.go tools/llgo/test/execution/functions/multivalue.go tools/llgo/test/execution/functions/unreachable.go tools/llgo/test/execution/go.go tools/llgo/test/execution/if/lazy.go tools/llgo/test/execution/init.go tools/llgo/test/execution/interfaces/assert.go tools/llgo/test/execution/interfaces/basic.go tools/llgo/test/execution/interfaces/comparei2i.go tools/llgo/test/execution/interfaces/comparei2v.go tools/llgo/test/execution/interfaces/e2i_conversion.go tools/llgo/test/execution/interfaces/embedded.go tools/llgo/test/execution/interfaces/error.go tools/llgo/test/execution/interfaces/i2i_conversion.go tools/llgo/test/execution/interfaces/import.go tools/llgo/test/execution/interfaces/methods.go tools/llgo/test/execution/interfaces/static_conversion.go tools/llgo/test/execution/interfaces/wordsize.go tools/llgo/test/execution/literals/array.go tools/llgo/test/execution/literals/func.go tools/llgo/test/execution/literals/map.go tools/llgo/test/execution/literals/slice.go tools/llgo/test/execution/literals/struct.go tools/llgo/test/execution/maps/delete.go tools/llgo/test/execution/maps/insert.go tools/llgo/test/execution/maps/lookup.go tools/llgo/test/execution/maps/range.go tools/llgo/test/execution/methods/methodvalues.go tools/llgo/test/execution/methods/nilrecv.go tools/llgo/test/execution/methods/selectors.go tools/llgo/test/execution/new.go tools/llgo/test/execution/nil.go tools/llgo/test/execution/operators/basics.go tools/llgo/test/execution/operators/binary_untyped.go tools/llgo/test/execution/operators/shifts.go tools/llgo/test/execution/slices/append.go tools/llgo/test/execution/slices/cap.go tools/llgo/test/execution/slices/compare.go tools/llgo/test/execution/slices/copy.go tools/llgo/test/execution/slices/index.go tools/llgo/test/execution/slices/literal.go tools/llgo/test/execution/slices/make.go tools/llgo/test/execution/slices/sliceexpr.go tools/llgo/test/execution/strings/add.go tools/llgo/test/execution/strings/bytes.go tools/llgo/test/execution/strings/compare.go tools/llgo/test/execution/strings/index.go tools/llgo/test/execution/strings/range.go tools/llgo/test/execution/strings/runetostring.go tools/llgo/test/execution/strings/slice.go tools/llgo/test/execution/structs/compare.go tools/llgo/test/execution/structs/embed.go tools/llgo/test/execution/switch/branch.go tools/llgo/test/execution/switch/default.go tools/llgo/test/execution/switch/empty.go tools/llgo/test/execution/switch/scope.go tools/llgo/test/execution/switch/strings.go tools/llgo/test/execution/switch/type.go tools/llgo/test/execution/types/named.go tools/llgo/test/execution/types/recursive.go tools/llgo/test/execution/unsafe/const_sizeof.go tools/llgo/test/execution/unsafe/offsetof.go tools/llgo/test/execution/unsafe/pointer.go tools/llgo/test/execution/unsafe/sizeof_array.go tools/llgo/test/execution/unsafe/sizeof_basic.go tools/llgo/test/execution/unsafe/sizeof_struct.go tools/llgo/test/execution/var.go tools/llgo/test/execution/varargs.go tools/llgo/test/gllgo/dead.go tools/llgo/test/irgen/mangling.go tools/llgo/test/lit.cfg tools/llgo/test/lit.site.cfg.in tools/llgo/update_third_party.sh tools/llgo/utils/benchcomp/README tools/llgo/utils/benchcomp/analyze.R tools/llgo/utils/benchcomp/main.go tools/llvm-go/llvm-go.go
llvm-commits mailing list
llvm-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
tools/llgo/test/execution/assignment/namedresult.go | ||
---|---|---|
2–5 | If I'm understanding this use of go run correctly, this seems like a very strange testing strategy for the LLVM project. It would be as though clang's tests used gcc or msvc as a testing tool and only tested the output of compiled programs. Is there a reason you haven't organized your testing like clang? I.e. verifying the compiler's IRGen and diagnostics? |
tools/llgo/test/execution/assignment/namedresult.go | ||
---|---|---|
2–5 | Most of the tests in test/execution are from an earlier stage of the project when it was more experimental. I agree with you -- at this point, there is no excuse for making them depend on the output of another compiler. I'll see if I can rewrite them to use FileCheck to check the command output. I also intend to gradually make these tests more focused and move them into the other subdirectories of test, but that will take time. |
- List supported platforms in README
- Filter libgo_*san targets based on available sanitizers
- FileCheck-ify the execution tests
tools/llgo/test/execution/assignment/namedresult.go | ||
---|---|---|
2–5 | Done. |
tools/llgo/README.TXT | ||
---|---|---|
18–19 | Could you add a brief high-level description of platform-specific code required? I.e., where should someone who wants to support other platforms start looking? |
So far it looks pretty good. First round of review.
These comments are mostly nitpicky stuff, and I don't think we want to address them in this patch.If we do, this patch will become a port from the git repostioy plus various different kind of fixes. I left these comments just because I noticed them and didn't want to lose it. If something needs to be addressed, I think it's better to fix in a follow-up patch.
tools/llgo/CMakeLists.txt | ||
---|---|---|
45 | Why do you use locally built clang to build go? Can't it compile with system's C/C++ compiler? | |
tools/llgo/build/context.go | ||
66 | The regular expressions were written as if they are surrounded by ^ and $, but they are actually not. As a result they look odd -- the trailing .* has no effect, and /k?/ in /k?freebsd/ doesn't have any effect. | |
tools/llgo/cmd/cc-wrapper/main.go | ||
52 | os.StartProcess is a low-level interface, and for this function it doesn't seems to be needed. We should just use os.Command instead. We may be able to replace this function with a few lines of os.Command calls. | |
tools/llgo/cmd/gllgo/gllgo.go | ||
50 | nit: just add one more \n to the last Printf instead of calling Println? | |
242 | So you check for errors for -I but not for -D and other options? | |
494 | This function could be simplified if you start with the empty byte array with extra buffer (make([]byte, 0, len(data)*4+10) and append strings using append. | |
tools/llgo/debug/debug.go | ||
388 | Is this TODO? | |
392 | ditto |
I decided to address the things that were easy to address. We can probably address the other items later.
tools/llgo/CMakeLists.txt | ||
---|---|---|
45 | It can (this is how the stage1 compiler is built), but these two targets (the stage2 and stage3 compilers) are used for bootstrapping, so they use previous compiler stages. | |
tools/llgo/README.TXT | ||
18–19 | Done. | |
tools/llgo/build/context.go | ||
66 | Ack. We might just want to remove most of these entries for now and let people add them back in as the compiler is ported to other platforms. | |
tools/llgo/cmd/cc-wrapper/main.go | ||
52 | Agreed. | |
tools/llgo/cmd/gllgo/gllgo.go | ||
50 | Done. | |
242 | Good catch, added checks for the other options. | |
494 | Done. | |
tools/llgo/debug/debug.go | ||
388 | Perhaps. I am not sure what the debug info for these should look like. @axw was the original author of this code. | |
392 | Ditto |
- Modify the update_third_party script to remove unused GPL licensed files
- Fix Linuxisms in update_third_party.sh script
- Add porting and contribution information to the README
- Address some of Rui's comments
- Use USES_TERMINAL with check-libgo target
tools/llgo/irgen/annotations.go | ||
---|---|---|
23 | Is this how gofmt sorted? | |
tools/llgo/irgen/cabi.go | ||
43 | This field doesn't seems to be used at all. Is this needed? | |
96 | Doesn't this have to include the padding after the last member? I mean if we have a struct, say struct { int; char; }, it's size would be calculated as 5 by this function, but I think it should be 8. (If it's 5, sizeof [2]struct{ int; char; } would become 10?) | |
104 | Oh okay, so the function is used only here. Then the thing I pointed out above is not actually a problem. | |
208 | Not very sure what this piece of code does... but is it going to create for example 1 million values if we have [1024 * 1024]sometype in source code? | |
tools/llgo/irgen/strings.go | ||
72 | Does this compare a value of type int as a pointer? stringiter2 returns (int, rune), not (*sometype, rune). Oh but this may be intentional -- currently, in all architectures sizeof(int) == sizeof(pointer). |
- Give signext/zeroext attributes to integer arguments according to width and signedness
- Re-gofmt
tools/llgo/irgen/annotations.go | ||
---|---|---|
23 | Reformatted (here and elsewhere). | |
tools/llgo/irgen/cabi.go | ||
43 | This field ought to be used to control whether certain arguments receive a signext or zeroext attribute, but it seems we've somehow gotten away with not doing this up to now. Fixed. | |
104 | Right. | |
208 | It shouldn't do. This function is only used when passing an argument directly (i.e. in registers), and with the System V x86-64 calling convention that can only happen with arguments of size <= 16 bytes. | |
tools/llgo/irgen/strings.go | ||
72 | See include/llvm/IR/IRBuilder.h line 1469 or thereabouts -- this will compare the given value against the null value of its type, which in this case would just be integer zero. |
tools/llgo/debug/debug.go | ||
---|---|---|
388 | Yes, sorry, please add a TODO/FIXME. This should be DW_TAG_pointer_type to __go_map. I ran gllgo over some basic code and the current output isn't great. I'll submit a patch or two once this is in to improve that. (Sorry, I wrote this comment shortly after seeing the above, didn't realise I hadn't submitted the reply.) | |
392 | ditto :) |
- Add MIT/UIUC dual licensed versions of runtime library headers,
- Build tweaks
- Add FIXMEs for debug info
So this should now be ready to check in to the newly created llgo subproject. I've uploaded my final changes here for the record and will close this revision.
I found there is files that mixed up with tab and spaces, is there any
coding style for go language?
Maybe use 2 space for all is a better idea?
Why do you use locally built clang to build go? Can't it compile with system's C/C++ compiler?