Page MenuHomePhabricator

eandrews (Elizabeth Andrews)
User

Projects

User does not belong to any projects.

User Details

User Since
Jul 10 2017, 11:37 AM (156 w, 6 d)

Recent Activity

Feb 12 2020

eandrews committed rGa58017e5cae5: Fix type-dependency of bitfields in templates (authored by eandrews).
Fix type-dependency of bitfields in templates
Feb 12 2020, 1:38 PM
eandrews closed D72242: Fix crash on value dependent bitfields in if conditions in templates.
Feb 12 2020, 1:38 PM · Restricted Project
eandrews added a comment to D72242: Fix crash on value dependent bitfields in if conditions in templates.

I'm not sure why Phabricator is still showing Needs Review, but @rnk's approval should make this count as accepted.

Feb 12 2020, 1:37 PM · Restricted Project
eandrews added a comment to D72242: Fix crash on value dependent bitfields in if conditions in templates.

Actually I just noticed it still says 'Needs review' above. I'm not sure what the protocol is. Can I push the patch?

Feb 12 2020, 1:10 PM · Restricted Project
eandrews added a comment to D72242: Fix crash on value dependent bitfields in if conditions in templates.

Thanks! I'll push it shortly. Just running a final ninja check-all after updating workspace.

Feb 12 2020, 11:47 AM · Restricted Project

Feb 6 2020

eandrews updated the diff for D72242: Fix crash on value dependent bitfields in if conditions in templates.

Thanks for taking a look Richard. This patch adds the required value-dependency check and sets type-dependency accordingly.

Feb 6 2020, 4:51 PM · Restricted Project

Jan 22 2020

eandrews added a comment to D72242: Fix crash on value dependent bitfields in if conditions in templates.
In D72242#1820908, @rnk wrote:

I think I liked the first version of this patch better. I would say, if the AST after instantiation remains the same as it was before D69950, then the first one seems like the right fix.

Jan 22 2020, 3:12 PM · Restricted Project

Jan 9 2020

eandrews updated the diff for D72242: Fix crash on value dependent bitfields in if conditions in templates.

Semantic analysis for value dependent conditions is now skipped as per Erich's comment. Patch adds an argument to CheckBooleanCondition to still do the required analysis for noexcept expressions.

Jan 9 2020, 11:39 AM · Restricted Project

Jan 8 2020

eandrews added a comment to D72242: Fix crash on value dependent bitfields in if conditions in templates.

Thanks for taking a look Erich!

Jan 8 2020, 8:09 AM · Restricted Project

Jan 5 2020

eandrews created D72242: Fix crash on value dependent bitfields in if conditions in templates.
Jan 5 2020, 7:39 PM · Restricted Project

Dec 5 2019

eandrews added a comment to D69950: Reapply "Fix crash on switch conditions of non-integer types in templates".

This (when reapplied in https://reviews.llvm.org/rG878a24ee244a24c39d1c57e9af2) broke compilation of code that earlier built fine. A reduced example:

namespace glslang {
class TPoolAllocator {
  void operator=(TPoolAllocator);
};
template <class> class a {
  TPoolAllocator *b;
  void c() { allocator = *b; }
  TPoolAllocator allocator;
};
} // namespace glslang

With this patch, some errors in templates are diagnosed earlier (i.e. does not wait till instantiation). Since 'allocator' and 'b' aren't dependent, I think this is a valid diagnosis. GCC throws an error on this code upon instantiation. https://godbolt.org/z/X9Y-Vy

The original non-reduced case does build fine with GCC though (I'm fairly sure). I can try to reduce one later that still builds with GCC but fails with current clang.

Dec 5 2019, 11:30 AM · Restricted Project
eandrews added a comment to D69950: Reapply "Fix crash on switch conditions of non-integer types in templates".

This (when reapplied in https://reviews.llvm.org/rG878a24ee244a24c39d1c57e9af2) broke compilation of code that earlier built fine. A reduced example:

namespace glslang {
class TPoolAllocator {
  void operator=(TPoolAllocator);
};
template <class> class a {
  TPoolAllocator *b;
  void c() { allocator = *b; }
  TPoolAllocator allocator;
};
} // namespace glslang
Dec 5 2019, 9:46 AM · Restricted Project

Dec 3 2019

eandrews committed rG878a24ee244a: Reapply "Fix crash on switch conditions of non-integer types in templates" (authored by eandrews).
Reapply "Fix crash on switch conditions of non-integer types in templates"
Dec 3 2019, 3:42 PM

Nov 19 2019

eandrews added a comment to D69950: Reapply "Fix crash on switch conditions of non-integer types in templates".

hence not throwing the warning on any platform?

The way I read the buildbot breakage, an existing ClangTidy test passed before and after this change, but broke on Windows. The breakage was that the warnings stopped being produced.

The existing test does not have this warning. I modified the test to add the check for this warning since it is generated on Linux after my patch. It is not generated on Windows because of delayed template parsing.

Is it just that warning that is not generated, or all warnings in that test file?

Nov 19 2019, 2:42 PM · Restricted Project
eandrews added a comment to D69950: Reapply "Fix crash on switch conditions of non-integer types in templates".

hence not throwing the warning on any platform?

The way I read the buildbot breakage, an existing ClangTidy test passed before and after this change, but broke on Windows. The breakage was that the warnings stopped being produced.

Nov 19 2019, 9:01 AM · Restricted Project

Nov 11 2019

eandrews added a comment to D69950: Reapply "Fix crash on switch conditions of non-integer types in templates".

The patch changes type dependency of field 'std::string s' and sets it based on field type (i.e. not type-dependent).

Nov 11 2019, 12:58 PM · Restricted Project

Nov 8 2019

eandrews added a comment to D69950: Reapply "Fix crash on switch conditions of non-integer types in templates".

Thank you for letting me know. The patch has been reverted.

Nov 8 2019, 2:33 PM · Restricted Project
eandrews added a comment to D69950: Reapply "Fix crash on switch conditions of non-integer types in templates".

@rnk @gribozavr2 clang-tidy test "bugprone-string-integer-assignment.cpp" caused a bot fail on windows. From the logs it looks like the 'CHECK-FIXES' I added in that test is failing because the fix isn't applied. I am not sure why. I don't have a windows build set up and I assume a new build is going to take some time. I am not sure what the protocol for this is. Should I go ahead and revert my patch while I investigate the issue on Windows or should I just push a new patch deleting the 'CHECK-FIXES' while I look into the issue?

Nov 8 2019, 11:21 AM · Restricted Project
eandrews updated subscribers of D69950: Reapply "Fix crash on switch conditions of non-integer types in templates".

Thank you for taking a look Reid and Dmitri. There were no fails with check-all.

Nov 8 2019, 9:58 AM · Restricted Project

Nov 7 2019

eandrews created D69950: Reapply "Fix crash on switch conditions of non-integer types in templates".
Nov 7 2019, 8:18 AM · Restricted Project

Aug 14 2019

eandrews added a comment to D61027: Fix crash on switch conditions of non-integer types in templates.

Thank you for letting me know. I did try reproducing the issue with check-all yesterday but was unable to. I do not think I build with 'clang-tools-extra' project enabled though. I'll take another look today.

Aug 14 2019, 7:16 AM · Restricted Project

Aug 13 2019

eandrews added a comment to D61027: Fix crash on switch conditions of non-integer types in templates.

Sorry, but this change broke ClangTidy tests: http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/16398. I reverted it.

Aug 13 2019, 12:20 PM · Restricted Project
eandrews committed rG76945821b9ca: Fix crash on switch conditions of non-integer types in templates (authored by eandrews).
Fix crash on switch conditions of non-integer types in templates
Aug 13 2019, 8:54 AM
eandrews committed rL368706: Fix crash on switch conditions of non-integer types in templates.
Fix crash on switch conditions of non-integer types in templates
Aug 13 2019, 8:54 AM
eandrews closed D61027: Fix crash on switch conditions of non-integer types in templates.
Aug 13 2019, 8:54 AM · Restricted Project

Aug 7 2019

eandrews added a comment to D61027: Fix crash on switch conditions of non-integer types in templates.

Thanks for taking a look Reid! I rebased the patch and had a conflict with one of Richard's patches and so ran the lit tests again and noticed there are several new failures. I am taking a look at those now.

Aug 7 2019, 7:22 AM · Restricted Project

Jul 29 2019

eandrews added a comment to D61027: Fix crash on switch conditions of non-integer types in templates.

ping

Jul 29 2019, 8:34 AM · Restricted Project

Jul 17 2019

eandrews added a comment to D61027: Fix crash on switch conditions of non-integer types in templates.

I just realized I modified the summary when I uploaded a new patch, but did not add a comment about the changes I made. Based on feedback from the bug report (https://bugs.llvm.org/show_bug.cgi?id=40982), I changed my approach for this crash. This patch changes the type dependency of the member which causes the crash.

Jul 17 2019, 3:12 PM · Restricted Project

Jun 10 2019

eandrews updated the diff for D61027: Fix crash on switch conditions of non-integer types in templates.
Jun 10 2019, 10:08 AM · Restricted Project

Apr 24 2019

eandrews added a comment to D61027: Fix crash on switch conditions of non-integer types in templates.

I ran the test you provided and it does throw errors without instantiation

Apr 24 2019, 2:22 PM · Restricted Project

Apr 23 2019

eandrews added a comment to D61027: Fix crash on switch conditions of non-integer types in templates.

I apologize for the confusion. I was working on an incorrect branch earlier, and so abandoned that patch and uploaded a new revision.

Apr 23 2019, 9:44 AM · Restricted Project
eandrews created D61027: Fix crash on switch conditions of non-integer types in templates.
Apr 23 2019, 9:31 AM · Restricted Project

Nov 6 2018

eandrews committed rL346237: [benchmark] Disable exceptions in Microsoft STL.
[benchmark] Disable exceptions in Microsoft STL
Nov 6 2018, 8:00 AM
eandrews closed D52998: [benchmark] Disable exceptions in Microsoft STL.
Nov 6 2018, 8:00 AM

Nov 2 2018

eandrews added a comment to D52998: [benchmark] Disable exceptions in Microsoft STL.

Thanks!

Nov 2 2018, 10:28 AM

Nov 1 2018

eandrews added a comment to D52998: [benchmark] Disable exceptions in Microsoft STL.

Upstreaming it first might be better, especially since the change seems to be trivial. Is this line addition the only change proposed for the benchmark library?

Yes this line is the only change.

Do you want me to submit the patch to the benchmark library? It seems that it would help to speedup the process.

That would be helpful. Thank you!

Nov 1 2018, 11:57 AM
eandrews updated the diff for D52998: [benchmark] Disable exceptions in Microsoft STL.

@kbobyrev I apologize if I was unclear in the comments. I was asking if the changes proposed in the comments are alright with you since they would involve modifying benchmark/CMakelists.txt (instead of llvm/CMakeLists.txt as discussed in mailing list). As Zachary mentioned in comments, _HAS_EXCEPTIONS should be set to 0 only when exceptions are disabled. Since exception handling for benchmarks is handled in benchmark/CMakeLists.txt, I think it makes most sense to add the definition there. I have now uploaded the proposed change for review.

Nov 1 2018, 10:37 AM

Oct 29 2018

eandrews added a comment to D52998: [benchmark] Disable exceptions in Microsoft STL.

Yes. I understand. At the moment, exception handling flags are generated based on BENCHMARK_ENABLE_EXCEPTIONS in utils/benchmark/CMakeLists.txt . However, _HAS_EXCEPTIONS is not defined based on this (code below). The warnings are a result of this mismatch.

if (NOT BENCHMARK_ENABLE_EXCEPTIONS)
    add_cxx_compiler_flag(-EHs-)
    add_cxx_compiler_flag(-EHa-)
  endif()

I think it makes most sense to add definition for _HAS_EXCEPTIONS=0 here as opposed to modifying llvm/CMakeLists.txt.

Then i'd recommend/suggest to submit this upstream first.

Please correct me if I'm wrong. I'm not too familiar with CMake. @kbobyrev Please let me know what you think as well since you had suggested llvm/CMakeLists.txt.

Oct 29 2018, 3:24 PM

Oct 25 2018

eandrews added a comment to D40925: Add option -fkeep-static-consts.

I think it makes sense to include this case just to make the flag more complete. Also, we don't really match GCC behavior for this flag. GCC emits all static constants by default (when O0). When optimized, this flag has no effect on GCC. The negation is used to not emit the constants.

Oct 25 2018, 11:26 AM

Oct 9 2018

eandrews added a comment to D52998: [benchmark] Disable exceptions in Microsoft STL.

Yes. I understand. At the moment, exception handling flags are generated based on BENCHMARK_ENABLE_EXCEPTIONS in utils/benchmark/CMakeLists.txt . However, _HAS_EXCEPTIONS is not defined based on this (code below). The warnings are a result of this mismatch.

Oct 9 2018, 9:06 AM

Oct 8 2018

eandrews added a comment to D52998: [benchmark] Disable exceptions in Microsoft STL.

Thanks for the information Zachary.

Oct 8 2018, 2:58 PM
eandrews added a comment to D52998: [benchmark] Disable exceptions in Microsoft STL.

I do not think defining _HAS_EXCEPTIONS=0 affects C++ exceptions in the application. I thought it only affected the STL. I will verify this and update you.

Oct 8 2018, 2:27 PM
eandrews created D52998: [benchmark] Disable exceptions in Microsoft STL.
Oct 8 2018, 1:54 PM

Sep 4 2018

eandrews abandoned D51545: Enable -Wtautological-unsigned-zero-compare under -Wextra.
Sep 4 2018, 7:06 AM
eandrews added a comment to D51545: Enable -Wtautological-unsigned-zero-compare under -Wextra.

Alright. Thanks for the review. I will abandon this patch

Sep 4 2018, 7:05 AM

Aug 31 2018

eandrews added a comment to D51545: Enable -Wtautological-unsigned-zero-compare under -Wextra.

@lebedev.ri is there a specific reason -Wtautological-unsigned-zero-compare was removed? All the issues I am aware of talks about comparisons with min/max.

Aug 31 2018, 8:58 AM
eandrews created D51545: Enable -Wtautological-unsigned-zero-compare under -Wextra.
Aug 31 2018, 8:51 AM

Aug 22 2018

eandrews committed rL340439: Currently clang does not emit unused static constants. GCC emits these.
Currently clang does not emit unused static constants. GCC emits these
Aug 22 2018, 12:06 PM
eandrews committed rC340439: Currently clang does not emit unused static constants. GCC emits these.
Currently clang does not emit unused static constants. GCC emits these
Aug 22 2018, 12:06 PM
eandrews closed D40925: Add option -fkeep-static-consts.
Aug 22 2018, 12:06 PM

Aug 20 2018

eandrews added a comment to D40925: Add option -fkeep-static-consts.
In D40925#1206416, @rnk wrote:

lgtm!

Aug 20 2018, 1:19 PM
eandrews added inline comments to D40925: Add option -fkeep-static-consts.
Aug 20 2018, 12:55 PM
eandrews updated the diff for D40925: Add option -fkeep-static-consts.

Based on Reid's feedback, I changed option to CodeGenOption

Aug 20 2018, 12:54 PM

Aug 17 2018

eandrews updated the diff for D40925: Add option -fkeep-static-consts.

This patch fell through the cracks earlier. I apologize. Based on Reid's and Erich's feedback, I am now adding the variable to @llvm.used. Additionally I modified the if statements to ensure only static variables are considered.

Aug 17 2018, 1:34 PM

Feb 14 2018

eandrews created D43321: Improve documentation for attribute artificial.
Feb 14 2018, 2:57 PM
eandrews added a comment to D40925: Add option -fkeep-static-consts.

I think we can use CodeGenModule::addUsedGlobal for this purpose. I noticed this is what attribute 'used' calls. I need to look into it though.

Feb 14 2018, 7:30 AM

Feb 13 2018

eandrews created D43259: Implement function attribute artificial.
Feb 13 2018, 2:46 PM

Jan 18 2018

eandrews added a comment to D40925: Add option -fkeep-static-consts.

That makes sense. Is it not possible to implement the required functionality using a flag vs an attribute? In an earlier comment you mentioned adding the global to @llvm.used to prevent GlobalDCE from removing it. Can you not do that when using a flag?

Jan 18 2018, 8:14 AM

Jan 10 2018

eandrews added a comment to D40925: Add option -fkeep-static-consts.

Thanks for the review Reid. Sorry for the delay in my response. I was OOO.

Jan 10 2018, 2:00 PM

Dec 11 2017

eandrews added a comment to D40925: Add option -fkeep-static-consts.

*ping*

Dec 11 2017, 8:33 PM

Dec 6 2017

eandrews created D40925: Add option -fkeep-static-consts.
Dec 6 2017, 3:24 PM

Nov 2 2017

eandrews added a comment to D39210: Add default calling convention support for regcall..

No problem! Thanks!

Nov 2 2017, 2:08 PM
eandrews added a comment to D39210: Add default calling convention support for regcall..

*ping*

Nov 2 2017, 12:47 PM

Oct 25 2017

eandrews added inline comments to D39210: Add default calling convention support for regcall..
Oct 25 2017, 11:00 AM
eandrews updated the diff for D39210: Add default calling convention support for regcall..

Updated patch to set default calling convention for main() in CheckMain()

Oct 25 2017, 10:58 AM

Oct 23 2017

eandrews created D39210: Add default calling convention support for regcall..
Oct 23 2017, 3:57 PM

Sep 25 2017

eandrews updated the diff for D36487: Emit section information for extern variables. .

I've modified the patch to emit a warning for re-declarations only. I also removed the isUsed check.

Sep 25 2017, 11:55 AM

Sep 21 2017

eandrews added inline comments to D36487: Emit section information for extern variables. .
Sep 21 2017, 10:16 AM
eandrews updated the diff for D36487: Emit section information for extern variables. .

I've updated the patch based on review comments. The changes include setting section unconditionally for extern variables and emitting a warning if section attribute is specified on a redeclaration.

Sep 21 2017, 10:05 AM

Aug 28 2017

eandrews added a comment to D36487: Emit section information for extern variables. .

*ping*

Aug 28 2017, 6:18 AM

Aug 22 2017

eandrews added a comment to D36487: Emit section information for extern variables. .

LangRef has been updated and accepted. You can find it here - https://reviews.llvm.org/rL311459

Aug 22 2017, 8:36 AM
eandrews updated the diff for D36712: Emit section information for extern variables.

Updated the documentation to explicitly state that section information for a variable can be explicit or inferred.

Aug 22 2017, 7:30 AM

Aug 16 2017

eandrews added a comment to D36712: Emit section information for extern variables.

The problem is that the mismatch between sections does not have to lead to any undesirable behavior. Whether it does or not depends on a particular case. I think we should be consistent though---if we decide that a mismatch between the section for a declaration and the section for a definition leads to an undefined behavior, then it should be so regardless of whether the sections were given explicitly or inferred.

Aug 16 2017, 11:59 AM
eandrews updated the diff for D36712: Emit section information for extern variables.

Updated the patch to include Krzysztof's comment about explicitly stating undefined behavior for section information mismatch in global variable declaration and definition. This should cover the case where section is explicitly specified in definition but not declaration.

Aug 16 2017, 11:44 AM
eandrews added a comment to D36712: Emit section information for extern variables.

In the cases when the section is explicitly given on a definition, it was likely imposed by something like the "section" attribute in the source. I don't think it's unreasonable to expect that the declarations (in the original source as well as in the generated IR) should carry that information as well. However, since clang has apparently been ignoring that attribute on declarations, it's been generating IR where declarations may not have sections, but the corresponding definitions do.

Aug 16 2017, 8:33 AM
eandrews updated the diff for D36712: Emit section information for extern variables.

Corrected spelling error.

Aug 16 2017, 7:48 AM

Aug 15 2017

eandrews added inline comments to D36712: Emit section information for extern variables.
Aug 15 2017, 1:16 PM
eandrews updated the diff for D36712: Emit section information for extern variables.

I updated the documentation to include Simon's comments. I wasn't sure whether to include the line about section information being retained in LLVM IR, since Eli mentioned it could vary for different front-ends. I have included it in this revision.

Aug 15 2017, 12:40 PM

Aug 14 2017

eandrews created D36712: Emit section information for extern variables.
Aug 14 2017, 1:58 PM
eandrews updated the diff for D36487: Emit section information for extern variables. .

As per review comments, removed extra RUNS in lit test.

Aug 14 2017, 1:54 PM

Aug 8 2017

eandrews added inline comments to D36487: Emit section information for extern variables. .
Aug 8 2017, 2:39 PM
eandrews created D36487: Emit section information for extern variables. .
Aug 8 2017, 2:05 PM

Jul 21 2017

eandrews updated the diff for D35259: Complex Long Double classification In RegCall calling convention.

Regcall-specific checks for Lin64 now occur only if CXXABI returns false. An existing test has also been modified to verify behavior with non trivial destructors.

Jul 21 2017, 6:51 AM

Jul 19 2017

eandrews updated the diff for D35259: Complex Long Double classification In RegCall calling convention.

As per revision comments, I moved the condition for extended precision floating type to isX86VectorTypeForVectorCall. This update will now alter behavior for complex long double type under vectorcall calling convention as well. Returns/parameters will be passed in memory.

Jul 19 2017, 10:58 AM

Jul 11 2017

eandrews added a comment to D35259: Complex Long Double classification In RegCall calling convention.
In D35259#805409, @rnk wrote:

Oren discovered this miss to the original implementation. I'd reviewed this internally quite a bit.

The reason for the Win32ABI test is that MSVC 'long double' is actually small enough for SSE registers in this case. I DO now (looking again, sorry Elizabeth) wonder if there is a better way to exclude extended-length LongDouble type? Could we us the 'length' of it instead? @rnk : opinion?

Yeah, you can ask clang::TargetInfo for the format of most basic FP types. The code to do that looks like:

&TI.getLongDoubleFormat() == &llvm::APFloat::x87DoubleExtended()

Any reason you can't just add that condition to isX86VectorTypeForVectorCall? I assume we don't want to pass x86_fp80s in SSE registers for vectorcall either, right? That would eliminate the need for the isRegCallReturnableHA helper and the IsWin32StructABI parameter, which is a poorly named variable.

It actually WOULD make sense to apply this to vectorcall as well, wouldn't it? I presumed we didnt want to change its behavior, however vectorcall is MSVC-only (other than us), so the long-double issue isn't a thing over there.@eandrews?

Jul 11 2017, 10:36 AM
eandrews created D35259: Complex Long Double classification In RegCall calling convention.
Jul 11 2017, 8:39 AM