- User Since
- Jan 2 2013, 4:34 PM (298 w, 4 d)
Wed, Sep 19
I ran the tests with your patch and everything passes, but of course, Timur mentioned that nothing in our test suite finds this issue. He probably encountered it in Chrome and it was hard to reduce into something we could add to the test suite.
Let's land this, I want to try it out. :)
Tue, Sep 18
- Fix one last use of setValueSubclassData
- Use private bitfield for subclass data
- Change cached order invalidation
- Add tests for removal and erasure
This is pretty cool. The process launching APIs in LLVM were pretty basic, left a lot to be desired, returned ints, etc etc. This addresses a lot of that.
Thanks, I think this looks good with a minor tweak to the test.
Mon, Sep 17
Needs tests, but I'm on board with this and the implementation.
I think we need to back up and think about what we're doing where and why.
Thanks! It's very likely we can use getenv("USERPROFILE") or the unicode equivalent to eliminate the remaining one. It's also pretty reasonable to just hardcode %USERPROFILE%/AppData/Local if we ever need this functionality.
Fri, Sep 14
@gbiv suggested that I test this on a large codebase, so I went ahead and built the Chrome unit_tests target with this, and the validation checks passed.
Thu, Sep 13
Right, sorry about that. :)
Wed, Sep 12
Any opinions on this? I'm eager to get it in so I can release it and get some faster builds, but I know it's a core data structure change.
I'm just waiting to hear from other sanitizer maintainers about their stance on adding the complexity of supporting mingw headers in the sanitizers.
Needs tests? Maybe they just aren't in the diff?
Tue, Sep 11
We should add old FPO data dumping next.
- use less ambiguous name
- pick a default CPU type for simplicity
Unfortunately, it looks like this changed the behavior of newlines embedded in arguments. When parsing response files, we want to treat \n as whitespace. When parsing a command line, CommandLineToArgvW apparently does not. The documentation says it separates arguments on "whitespace characters", but it doesn't say what it considers to be whitespace. Apparently \n is not whitespace.
- Be more string-y
Mon, Sep 10
I like the code, but I think this needs more tests, and better testing infrastructure before we can commit it. I would recommend beefing up COFFDumper::printUnwindInfo in llvm-readobj to print the ARM64 unwind opcodes in a human readable format, and then you won't need to write tests with hexadecimal CHECKs like you have now.
Sounds good, it could use an IR test though.
ಠ_ಠ mm_malloc.h, you are a source of much complexity.
I should add more reviewers, since adding a mingw port is not a small matter. I *think* that some of the sanitizer_common code already builds with mingw gcc for tsan-go, right? So, adding support for building with gcc on Windows should not be too onerous for the project as a whole. At least, that's what I think. Other folks can chime in.
Actually +@zturner this time.
Does it build with GCC, clang, or both?
lgtm with a tweaked comment. Thanks!
Fri, Sep 7
- beef up test
Passing the source location through to MCStreamer seems like a good idea. Before rL315264, this code used to use report_fatal_error. My goal was to make it less crummy, and make sure errors get diagnosed when emitting assembly or emitting object files.
I see that we've done this before in rL310419. This was originally added in rL275180. It'd be nice to figure out what tools generate this nop so we can enumerate all the patterns, but for now let's just do this.
I looked at the documentation for the cmake install command, but I still have no idea what a component is or what effect this option has. Can you elaborate on that?
lgtm, thanks! I thought that was going to be more of a pain. :)
Wow, three different ways to set the linker. I'm impressed with how many cooks there are in our kitchen.
I'd suggest fixing this in MCStreamer::EmitCFIStartProc by having it recover in some way that doesn't lead to future assertions. Also, it should be possible to write a standalone .s test case for this.
Thu, Sep 6
BTW, it's possible that this is also what's causing our size regression vs. non-LTO builds. If archives add lazy symbols which are marked as being used from a regular object, that might prevent us from discarding them.