smeenai (Shoaib Meenai)
User

Projects

User does not belong to any projects.

User Details

User Since
Aug 19 2016, 10:21 AM (65 w, 2 d)

Recent Activity

Fri, Nov 17

smeenai added a comment to D40197: Merge .xdata into .rdata by default.

Ah all right, that makes sense then.

Fri, Nov 17, 4:04 PM
smeenai added a comment to D40197: Merge .xdata into .rdata by default.

What are the advantages to the section merging, apart from matching link's ouput? I personally prefer LLD keeping the separate sections, since it's easier to examine them that way.

Fri, Nov 17, 3:48 PM

Tue, Nov 14

smeenai added a comment to D40025: [LLD] [COFF] Don't write long section names for sections that will be mapped at runtime.
In D40025#924844, @rnk wrote:

I wonder if we should limit the long section name extension to sections that aren't mapped at runtime. This makes sense because if some tool cares about unmapped section contents, they're going to need to look at the file on disk, not the image in memory, and that will have a full symbol table.

Does that make sense, or should we just go with this?

Tue, Nov 14, 10:45 AM

Mon, Nov 13

smeenai added a comment to D39821: Add CLANG_DEFAULT_OBJCOPY to allow Clang to use llvm-objcopy for dwarf fission.

It wasn't automatically closed because the commit (https://reviews.llvm.org/rL317960) referenced the wrong differential in its Differential Revision field. Not sure how that happened.

Mon, Nov 13, 2:44 PM

Sun, Nov 12

smeenai added a comment to D39462: [Sema] Implement -Wmaybe-tautological-constant-compare for when the tautologicalness is data model dependent.

I don't speak for the entire project, but I'm not sure I'm interested in the diagnostic you're actually offering to contribute here. It may produce a warning on your specific test case, but I think it's really much too rigid and will lead to massive false positives. I sketched the basics of a design that I think I could accept; if you don't want to implement it, that's your right, but that doesn't make me more likely to accept what you're willing to implement.

Just to reiterate that we are talking about the same thing here:

  • D38101 is already merged. -Wtautological-constant-compare is here.
  • There are cases when it warns for some target platform, but not the other, as complained in D39149, and post-review mails for D38101
  • So far it seems all the cases reduce to ` #include <limits> #include <cstdint> int main() { using T1 = long; using T2 = int;

    T1 r; if (r < std::numeric_limits<T2>::min()) {} if (r > std::numeric_limits<T2>::max()) {} } `
  • *This* differential (D39462) would find such cases, and issue them under different diagnostic, thus reducing the "false-positive" (it is an open question whether they are actual false-positives or not) rate of -Wtautological-constant-compare.

I think you might have this backwards. We think of the check for the diagnostic as being the test, so a false positive is when we warn when we shouldn't. What you've identified here is a false *negative*, i.e. a case where you believe it should warn (because it would warn on a different target) that it currently does not.

Sun, Nov 12, 12:05 PM · Restricted Project

Thu, Nov 9

smeenai accepted D39814: Create a Cross-Compilation CMake toolchain file for NonWindows -> Windows.

Awesome!

Thu, Nov 9, 12:25 PM

Wed, Nov 8

smeenai added a comment to D39814: Create a Cross-Compilation CMake toolchain file for NonWindows -> Windows.

Thanks for productionizing this! It'll be awesome to have it in-tree.

Wed, Nov 8, 1:08 PM

Tue, Nov 7

smeenai added a comment to D39716: Explicitly set CMake policy CMP0068 to NEW to avoid warnings.

This seems fine as far as avoiding the warnings themselves go, but I'm not familiar enough with install_name and rpath on macOS to know if the new policy behavior is doing the right thing.

Tue, Nov 7, 9:33 AM

Fri, Nov 3

smeenai added inline comments to D39538: [llvm-ar] Support an options string that start with a dash.
Fri, Nov 3, 12:50 PM

Thu, Nov 2

smeenai committed rL317272: [tools] Add option to install binutils symlinks.
[tools] Add option to install binutils symlinks
Thu, Nov 2, 2:44 PM
smeenai closed D39530: [tools] Add option to create binutils symlinks by committing rL317272: [tools] Add option to install binutils symlinks.
Thu, Nov 2, 2:44 PM
smeenai updated the diff for D39530: [tools] Add option to create binutils symlinks.

Rename from CREATE to INSTALL

Thu, Nov 2, 2:42 PM
smeenai added a comment to D39530: [tools] Add option to create binutils symlinks.

I'm wondering if LLVM_INSTALL_BINUTILS_SYMLINKS would be a better name. I went with "create" instead of "install" initially because this option will create the symlinks in the build tree in addition to the install tree, and "install" doesn't convey that fully, but I think most people would care a lot more about the installation part.

Thu, Nov 2, 1:46 PM
smeenai updated the diff for D39530: [tools] Add option to create binutils symlinks.

Add option and documentation

Thu, Nov 2, 1:45 PM
smeenai committed rL317264: [cmake] Remove policy conditionals.
[cmake] Remove policy conditionals
Thu, Nov 2, 1:34 PM
smeenai closed D39442: [cmake] Remove policy conditionals by committing rL317264: [cmake] Remove policy conditionals.
Thu, Nov 2, 1:34 PM
smeenai added a comment to D39540: [llvm-nm] Print 'I' for import table data in COFF.

GNU binutils nm also flags all of the .idata$X chunks as 'i' (while this patch only makes it set on .idata$2 and .idata$6) and also flags impfunction as 'I'.

Thu, Nov 2, 9:32 AM
smeenai requested changes to D39539: [llvm-nm] Don't error out on multiple occurrances of the -g/--external-only flag.

I'm not super familiar with the option parser by any means, but I think changing specific flags to be ZeroOrMore rather than Optional is much better than doing a global change for the behavior of Optional.

Thu, Nov 2, 9:08 AM
smeenai added a comment to D39538: [llvm-ar] Support an options string that start with a dash.

Not sure of a better implementation strategy, but I am a fan of adding support for the dashed variants, since I've hit the issue as well in the past.

Thu, Nov 2, 8:59 AM
smeenai added a comment to D39541: [LLD] [MinGW] Output debug info by default, unless the -s parameter is passed.

lld-link will include debug info sections in the linked binary if you pass -debug (and you probably also want to pass -nopdb to prevent PDB emission if you're using DWARF). Perhaps you should make the MinGW driver pass those two options by default and then not pass them if -s is passed to it?

Thu, Nov 2, 8:34 AM
smeenai added a comment to D39530: [tools] Add option to create binutils symlinks.

@jakehehrlich I see you got that Herald rule set up :)

Thu, Nov 2, 12:11 AM
smeenai created D39530: [tools] Add option to create binutils symlinks.
Thu, Nov 2, 12:10 AM

Wed, Nov 1

smeenai committed rL317188: [libclang] Add dummy libclang-headers target.
[libclang] Add dummy libclang-headers target
Wed, Nov 1, 10:04 PM
smeenai closed D39524: [libclang] Add dummy libclang-headers target by committing rL317188: [libclang] Add dummy libclang-headers target.
Wed, Nov 1, 10:04 PM
smeenai committed rL317187: [clangd] Remove redundant install.
[clangd] Remove redundant install
Wed, Nov 1, 10:02 PM
smeenai committed rL317155: [clang-tidy] Clean up installation rules.
[clang-tidy] Clean up installation rules
Wed, Nov 1, 6:48 PM
smeenai added a reviewer for D39524: [libclang] Add dummy libclang-headers target: akyrtzi.
Wed, Nov 1, 6:22 PM
smeenai updated the diff for D39524: [libclang] Add dummy libclang-headers target.

Better comment

Wed, Nov 1, 6:14 PM
smeenai committed rL317150: [clang-rename] Use add_clang_tool.
[clang-rename] Use add_clang_tool
Wed, Nov 1, 6:12 PM
smeenai closed D39522: [clang-rename] Use add_clang_tool by committing rL317150: [clang-rename] Use add_clang_tool.
Wed, Nov 1, 6:12 PM
smeenai committed rL317149: [clang-reorder-fields] Switch to add_clang_tool.
[clang-reorder-fields] Switch to add_clang_tool
Wed, Nov 1, 6:10 PM
smeenai closed D39523: [clang-reorder-fields] Switch to add_clang_tool by committing rL317149: [clang-reorder-fields] Switch to add_clang_tool.
Wed, Nov 1, 6:10 PM
smeenai committed rL317148: [cmake] Switch FATAL_ERROR to SEND_ERROR.
[cmake] Switch FATAL_ERROR to SEND_ERROR
Wed, Nov 1, 6:08 PM
smeenai created D39524: [libclang] Add dummy libclang-headers target.
Wed, Nov 1, 6:05 PM
smeenai created D39523: [clang-reorder-fields] Switch to add_clang_tool.
Wed, Nov 1, 5:52 PM
smeenai created D39522: [clang-rename] Use add_clang_tool.
Wed, Nov 1, 5:43 PM
smeenai added a comment to D39509: Vary Windows toolchain selection by -fuse-ld.

Also title says -fused-ld instead of -fuse-ld

Wed, Nov 1, 2:47 PM
smeenai added inline comments to D39509: Vary Windows toolchain selection by -fuse-ld.
Wed, Nov 1, 2:47 PM

Tue, Oct 31

smeenai added a comment to D39462: [Sema] Implement -Wmaybe-tautological-constant-compare for when the tautologicalness is data model dependent.

This solves the issue in D39149, at least.

Tue, Oct 31, 2:18 PM · Restricted Project
smeenai abandoned D39149: [libc++] Prevent tautological comparisons.

I confirmed that these warnings go away with D39462 applied, and they reappear if I manually specify -Wmaybe-tautological-constant-compare. Thank you!

Tue, Oct 31, 2:18 PM

Mon, Oct 30

smeenai created D39442: [cmake] Remove policy conditionals.
Mon, Oct 30, 6:56 PM
smeenai committed rL316972: [cmake] Make check_linker_flags operate via linker flags.
[cmake] Make check_linker_flags operate via linker flags
Mon, Oct 30, 6:31 PM
smeenai closed D39431: [cmake] Make check_linker_flags operate via linker flags by committing rL316972: [cmake] Make check_linker_flags operate via linker flags.
Mon, Oct 30, 6:31 PM
smeenai added inline comments to D39431: [cmake] Make check_linker_flags operate via linker flags.
Mon, Oct 30, 6:30 PM
smeenai created D39431: [cmake] Make check_linker_flags operate via linker flags.
Mon, Oct 30, 1:50 PM
smeenai added a comment to D39149: [libc++] Prevent tautological comparisons.

I'm thinking you could account for all possible type sizes in the existing (enabled by default) warning, and have a different warning for possibly tautological comparisons. E.g. if a long is being compared against INT_MAX, you know that's only tautological on some platforms, so it should go under -Wpossible-tautological-constant-compare (which would only be enabled by -Weverything and not -Wall or -Wextra); -Wtautological-constant-compare should be reserved for definitely tautological cases.

This sounds like a very promising direction. (That is: if the types of the values being compared are different, but are of the same size, then suppress the warning or move it to a different warning group that's not part of -Wtautological-compare.) That would also suppress warnings for cases like 'ch > CHAR_MAX', when ch is a char, but we could detect and re-enable the warning for such cases by, say, always warning if the constant side is an integer literal.

Mon, Oct 30, 1:39 PM
smeenai added a comment to D39149: [libc++] Prevent tautological comparisons.
  • The fact that under *different* circumstances the comparison is not tautological is not a false-positive.
Mon, Oct 30, 11:40 AM
smeenai added a comment to D39149: [libc++] Prevent tautological comparisons.

I'm thinking you could account for all possible type sizes in the existing (enabled by default) warning, and have a different warning for possibly tautological comparisons. E.g. if a long is being compared against INT_MAX, you know that's only tautological on some platforms, so it should go under -Wpossible-tautological-constant-compare (which would only be enabled by -Weverything and not -Wall or -Wextra); -Wtautological-constant-compare should be reserved for definitely tautological cases.

Mon, Oct 30, 11:29 AM
smeenai planned changes to D39149: [libc++] Prevent tautological comparisons.
Mon, Oct 30, 10:44 AM
smeenai added a comment to D39149: [libc++] Prevent tautological comparisons.

I dislike this change fairly strongly.

Mon, Oct 30, 10:43 AM

Sun, Oct 29

smeenai added a comment to D39406: Merge SymbolBody and Symbol into one class, SymbolBody..

I assume a corresponding change will also be made for COFF afterward?

Sun, Oct 29, 10:29 AM

Fri, Oct 27

smeenai added a comment to D39149: [libc++] Prevent tautological comparisons.

Like I said, not a huge fan of this change, but also not a huge fan of running into clang diagnostics (especially since we build with -Werror downstream).

Fri, Oct 27, 4:21 PM
smeenai added a comment to D39389: [MS] Allow access to ambiguous, inaccessible direct bases.

When you say "latest Windows SDK headers", do you mean SDK version 10.0.16299.15?

Fri, Oct 27, 2:35 PM
smeenai added a comment to D34692: [LLD][ELF] Add support for multiple passes to createThunks().

Thank you for being flexible on these, @ruiu! It makes our life a lot easier downstream :)

Fri, Oct 27, 8:46 AM

Thu, Oct 26

smeenai added a comment to D39317: Use -fuse-init-array if no gcc installation is found..

And lld currently silently mislinks inputs with .ctors sections

Thu, Oct 26, 9:28 AM

Wed, Oct 25

smeenai added a comment to D34692: [LLD][ELF] Add support for multiple passes to createThunks().

At the developer meeting @peter.smith had said the thunks implementation could be moved out into its own file provided one relocation function was available to it. If it's easy enough to do, perhaps we want to do that before landing, to alleviate the concerns around the relocations code becoming even more complicated?

Wed, Oct 25, 7:54 PM
smeenai committed rL316594: [cmake] Restrict resource file usage to Windows build hosts.
[cmake] Restrict resource file usage to Windows build hosts
Wed, Oct 25, 10:12 AM
smeenai closed D39265: [cmake] Restrict resource file usage to Windows build hosts by committing rL316594: [cmake] Restrict resource file usage to Windows build hosts.
Wed, Oct 25, 10:12 AM

Tue, Oct 24

smeenai created D39265: [cmake] Restrict resource file usage to Windows build hosts.
Tue, Oct 24, 5:22 PM
smeenai committed rL316502: [COFF] Add support for /WX.
[COFF] Add support for /WX
Tue, Oct 24, 2:19 PM
smeenai closed D39148: [COFF] Add support for /WX by committing rL316502: [COFF] Add support for /WX.
Tue, Oct 24, 2:19 PM
smeenai committed rL316501: [COFF] Clean up boolean flag handling.
[COFF] Clean up boolean flag handling
Tue, Oct 24, 2:17 PM
smeenai closed D39216: [COFF] Clean up boolean flag handling by committing rL316501: [COFF] Clean up boolean flag handling.
Tue, Oct 24, 2:17 PM

Mon, Oct 23

smeenai updated the diff for D39148: [COFF] Add support for /WX.

Rebase atop D39216

Mon, Oct 23, 4:47 PM
smeenai created D39216: [COFF] Clean up boolean flag handling.
Mon, Oct 23, 4:35 PM
smeenai requested changes to D39200: Fix an implicit conversion truncation.

https://reviews.llvm.org/rL316365 does this already.

Mon, Oct 23, 1:38 PM
smeenai added a comment to D38704: [libunwind] Abstract rwlocks into a class, provide a SRW lock implementation for windows.

BTW LLVM requires a minimum of Windows 7 according to https://releases.llvm.org/3.8.0/docs/ReleaseNotes.html, so you can rely on SRW locks always being present. If LLVM still has a fallback path for when SRW locks are absent, that could be cleaned up.

Mon, Oct 23, 10:34 AM
smeenai added inline comments to D39148: [COFF] Add support for /WX.
Mon, Oct 23, 9:15 AM
smeenai added a comment to D38513: [LLD] [RFC] [COFF] Add support for GNU binutils import libraries.

The grouping by import library is definitely important for both Microsoft-style and binutils-style import libraries. I'm pretty sure the second issue (how to get the correct ILT and IAT start addresses inside d000000.o) is solved by them special casing imports handling, as I mentioned before.

Mon, Oct 23, 8:57 AM

Oct 20 2017

smeenai created D39149: [libc++] Prevent tautological comparisons.
Oct 20 2017, 4:29 PM
smeenai created D39148: [COFF] Add support for /WX.
Oct 20 2017, 3:47 PM
smeenai added a comment to D37182: [libcxx] Special visibility macros for the experimental library.

Sorry, this has been on my queue for a long time, but I haven't gotten the chance to get to it yet. I'll try to take a look (at this and your other patches) in the next few days.

Oct 20 2017, 12:16 PM

Oct 16 2017

smeenai committed rL315964: [ExecutionEngine] Correct the size of a write in a COFF i386 relocation.
[ExecutionEngine] Correct the size of a write in a COFF i386 relocation
Oct 16 2017, 6:41 PM
smeenai closed D38872: [ExecutionEngine] Correct the size of a write in a COFF i386 relocation by committing rL315964: [ExecutionEngine] Correct the size of a write in a COFF i386 relocation.
Oct 16 2017, 6:41 PM

Oct 15 2017

smeenai added a comment to D38940: Make x86 __ehhandler comdat if parent function is.

You can use /Gy (or -ffunction-sections if you're not using the cl-style driver) to force each function into its own COMDAT section.

Oct 15 2017, 9:39 PM

Oct 13 2017

smeenai added inline comments to D38239: [ELF] - Define linkerscript symbols early..
Oct 13 2017, 12:52 PM

Oct 12 2017

smeenai committed rL315675: [LLD] Fix typo. NFC.
[LLD] Fix typo. NFC
Oct 12 2017, 11:11 PM
smeenai added a comment to D38513: [LLD] [RFC] [COFF] Add support for GNU binutils import libraries.

I looked into this some more. MinGW import libraries don't appear to contain the null directory table entry that's supposed to terminate the import directory table. This doesn't cause usually cause problems for link.exe, because you'll probably be linking against a non-MinGW import library, which provides the null entry (as .idata$3). Even if you're only linking against a MinGW import library, it looks like the loader doesn't require the entire terminating entry to be null; if either the Name RVA or the IAT RVA is null, the entry is considered to be a terminator. You can craft examples which cause problems though. All of the below discussion assumes 32-bit VS tools and MinGW, for reasons that will become apparent later.

Oct 12 2017, 10:52 PM
smeenai added a comment to D38872: [ExecutionEngine] Correct the size of a write in a COFF i386 relocation.

I can commit it for you, but I remembered that this should probably include a test case, assuming that there are tests set up for this functionality (and I hope there are).

Oct 12 2017, 7:15 PM
smeenai added a comment to D38871: [ExecutionEngine] Modify static_casts to not be tautological in some COFF i386 relocations.

I'd wait for @compnerd to take a look before acting on my comments, since I don't have much context on this code.

Oct 12 2017, 7:08 PM
smeenai accepted D38872: [ExecutionEngine] Correct the size of a write in a COFF i386 relocation.

Looks like a pretty simple and obvious fix.

Oct 12 2017, 6:47 PM
smeenai added a comment to D38869: [ExecutionEngine] Correct the size of a write in a COFF i386 relocation.

(as in, create a new differential. Adding llvm-commits to this one won't trigger an email with the patch contents, I believe.)

Oct 12 2017, 6:30 PM
smeenai requested changes to D38869: [ExecutionEngine] Correct the size of a write in a COFF i386 relocation.

Looks like a pretty small and obvious fix to me. However, you should reupload this patch and add llvm-commits as a subscriber, so that an email gets sent out to the mailing list.

Oct 12 2017, 6:29 PM
smeenai committed rL315626: [lld] Fix typo. NFC.
[lld] Fix typo. NFC
Oct 12 2017, 2:52 PM
smeenai added a comment to D38513: [LLD] [RFC] [COFF] Add support for GNU binutils import libraries.

My hypothesis is that link might special-case zero-sized sections, but I have no evidence to back that up yet.

I don't think that's it. Consider linking two separate GNU import libraries in the same module; if just piling up section chunks into .idata$5 as they are referenced, you'd have an IAT with entries mixed from both. So some sort of grouping per import library is needed, but the exact conditions for it aren't really known. Perhaps only ordering .idata$* chunks like this, but not other ones?

Oct 12 2017, 12:07 PM
smeenai updated subscribers of D38513: [LLD] [RFC] [COFF] Add support for GNU binutils import libraries.

Also adding more Windows people in case they have any ideas.

Oct 12 2017, 10:42 AM
smeenai added a comment to D38513: [LLD] [RFC] [COFF] Add support for GNU binutils import libraries.

Thanks for the detailed explanation! I'm pretty familiar with how short import libraries work, but I hadn't looked into MinGW-style import libraries too much before.

Oct 12 2017, 10:40 AM

Oct 11 2017

smeenai added a comment to D38513: [LLD] [RFC] [COFF] Add support for GNU binutils import libraries.

Could you elaborate on "the first object file in the static import library to be included is the function itself"? Is the function in question the linker-provided import thunk, or something else?

Oct 11 2017, 2:28 PM

Oct 9 2017

smeenai committed rL315234: [libc++] Support Microsoft ABI without vcruntime headers.
[libc++] Support Microsoft ABI without vcruntime headers
Oct 9 2017, 12:27 PM
smeenai closed D38522: [libc++] Support Microsoft ABI without vcruntime headers by committing rL315234: [libc++] Support Microsoft ABI without vcruntime headers.
Oct 9 2017, 12:27 PM
smeenai accepted D38522: [libc++] Support Microsoft ABI without vcruntime headers.

I spoke to @EricWF on IRC last week and got his approval to commit this without review if no one had gotten to it in a couple of days, so I'm going ahead with that.

Oct 9 2017, 12:26 PM
smeenai added a reviewer for D38681: [LLD] [COFF] Don't error out on relocations to discarded sections in .eh_frame: rnk.
Oct 9 2017, 9:03 AM

Oct 6 2017

smeenai added a comment to D38652: Use error() instead of warn() to report undefined symbols..

I'd been meaning to complain about lld-link outputting "warning" instead of "error" for undefined symbols. Thanks for fixing :)

Oct 6 2017, 4:17 PM

Oct 5 2017

smeenai added a comment to D38599: Remove warnings for dynamic_cast fallback..

Does dlopen cause issues even with RTLD_GLOBAL?

Oct 5 2017, 2:41 PM
smeenai added a comment to D34993: Refine our --wrap implementation.

Context is missing again.

Oct 5 2017, 9:50 AM

Oct 4 2017

smeenai committed rL314965: [libc++] Clarify names of ABI forcing macros.
[libc++] Clarify names of ABI forcing macros
Oct 4 2017, 7:20 PM
smeenai updated subscribers of D31363: [libc++] Remove cmake glob for source files.

@rjmccall, this adds a libc++ build dependency on LLVM's cmake modules. There were some issues when @zturner had added a similar dependency for his work on lit. To confirm, Apple still cares about the use case of building libc++ without having any LLVM cmake modules available (either from source or from an LLVM installation), right?

Oct 4 2017, 6:49 PM
smeenai committed rL314950: [libc++] Move cache variable definition. NFC.
[libc++] Move cache variable definition. NFC
Oct 4 2017, 4:53 PM
smeenai committed rL314949: [libc++] Allow users to explicitly specify ABI.
[libc++] Allow users to explicitly specify ABI
Oct 4 2017, 4:46 PM
smeenai committed rL314946: [libc++] Add site config option for ABI macros.
[libc++] Add site config option for ABI macros
Oct 4 2017, 4:19 PM