- User Since
- Oct 8 2012, 9:19 AM (424 w, 15 h)
Should this be using FileCheck (check for the thing you want, and check-not that it doesn't appear otherwise) instead of grep, perhaps?
Fri, Nov 20
Generally looks good to me, thanks!
Test coverage? (I guess all tests are already updated, but having a test showing what happens/testing the errors if these things are missing would probably be good)
Thu, Nov 19
Wed, Nov 18
Is it possible for the test case to be written in assembly and assembled during the test execution? (I realize a lot of the symbolizer tests are not written this way, but I think it's the direction we're generally moving in with these sort of tests) - good to still include the C source too, as you have.
(minor drive-by comment, but I'm not the right person to do more in depth review of this patch, unfortunately)
Tue, Nov 17
Looks good, thanks!
Thanks for the walkthroughs/help. Also stared at the code a bit. I think I get it now. Some of the confusion also came from having both LPM and NPM versions of the always inliner in the same file, though they seem to share no code.
Mon, Nov 16
Can you add a test case for this change?
Sounds good to me (I guess ALLOC and EXECINSTR can both be enabled separately? Is this consistent with the Binutils change? (or does one subsume the other?))
The new behavior seems reasonable and matches GNU as 2.36 https://sourceware.org/bugzilla/show_bug.cgi?id=26850
Performing the mandatory inlinings first simplifies the problem the full inliner needs to solve
Fri, Nov 13
Wed, Nov 11
I'm a bit confused - looks like there's a bunch of other uses of %llc_dwarf in this test directory - any idea why those ones don't fail in the same way/for you?
because FileCheck won't accept more than one occurrence of --allow-unused-prefixes
Tue, Nov 10
Does this mean, for instance, that sizeof(int*) is different from sizeof(void(*)())?
Mon, Nov 9
Fri, Nov 6
Looks OK - though you might want to check with -v output or otherwise check the addresses maybe? It might be the llvm-dwarfdump output, even with -v, is a bit hard to test what you may want to test on an object file - validating which line table entries are referring to which sections. If you want to test at that level to ensure the portions of the line table refer to the right bb sections, it might involve enhancing llvm-dwarfdump -v -debug-line output to include the name of the section (& section number, in cases where the name may not be unique (-fno-unique-section-names)) in a similar way to llvm-dwarfdump's output for debug_ranges/loc/etc.
Generally looks good, thanks!
Thu, Nov 5
Looks good - thanks!
Thanks for taking a look at this!
Maybe these would be more effective as generic range-based functions? (is "append(begin, end)" part of the container concept? Not 100% clear to me on a quick check) - would make it usable with a variety of containers in one go instead of adding it to individual containers (& not being able to add it to standard containers)?
Good for me - please wait for @mclow.lists to weigh in/close the loop as well, though.
How's this compare to the similar checks for variable templates? Is there some code/checking we could share here?
Wed, Nov 4
Sounds good to me then!
Looks good to me!
Seems pretty reasonable to me - @asbirlea could you check the test case to see the expected output, etc, makes sense? MSSA itself is not so much my wheelhouse so I'm not rightly sure myself.
Since the same code is used to mangle all these things, probably just test one of them?
Tue, Nov 3
Needs some test coverage?
Does Chromium need this fixed in clang? Or if it were fixed in libc++ would that be adequate? (does Chromium's build need to work with old libc++s, or does it always build with a libc++ that matches the compiler? (in the latter case, a fix in libc++ would be as good as a fix in clang))
Mon, Nov 2
(FWIW, I'd personally be in favor of -funique-section-names applying here - the specific textual description of the flag in documentation I don't think should necessarily be held as definitive, but possibly descriptive (perhaps it describes correctly the behavior as it exists/was implemented, and could be updated to include this new behavior) - seems like if someone's passing that flag it's probably because they don't want or may not have toolchain support for non-unique section names. It'd be pretty quirky/odd situation where they didn't want unique section names only for some sections and not for others.