fix: EXLR flagged as error by plugin #121

SWETAS04 · 2021-04-19T14:23:22Z

This PR incudes a new operand type which accepts either a relocatable symbol or absolute immediate value( with a warning).
Other Instructions which suffer from the same problem are also changed.

Signed-off-by: Sweta Shah <swetas04@gmail.com>

Signed-off-by: Sweta Shah <swetas04@GMAIL.COM>

…bols

Signed-off-by: Sweta Shah <swetas04@GMAIL.COM>

slavek-kucera · 2021-04-21T08:36:53Z

parser_library/src/diagnostic.cpp

+{
+    return diagnostic_op(diagnostic_severity::warning,
+        "D031",
+        " Operand value must be relocatable symbol with instruction " + instr,


I think it would be more useful to report the operand we're generating the warning for instead of the instruction name + replace must with should as this is a warning.

slavek-kucera · 2021-04-21T08:38:28Z

parser_library/src/diagnostic.h

@@ -439,6 +439,7 @@ struct diagnostic_op
    static diagnostic_op error_D023(const range& range);
    static diagnostic_op error_D024(const range& range, const std::string& type);
    static diagnostic_op warn_D025(const range& range, const std::string& type, const std::string& modifier);
+    static diagnostic_op warn_D031(const range& range, const std::string& modifier);


reorder, please.

Please add a test generating this message.

slavek-kucera · 2021-04-21T08:41:40Z

parser_library/src/diagnostic.h

@@ -457,20 +458,20 @@ struct diagnostic_op

    static diagnostic_op error_M112(const std::string& instr_name, const range& range);

-    static diagnostic_op error_M113(const std::string& instr_name, const range& range);


I think we should keep the original message ids, the messages relate to basically the same issues as before.

slavek-kucera · 2021-04-21T08:46:15Z

parser_library/src/checking/instr_operand.cpp

@@ -287,6 +287,13 @@ bool one_operand::check(
        }
        return true;
    }
+    // simple operand with relocatable symbol or immediate value
+
+    if (to_check.identifier.type == machine_operand_type::RELOC_IMM && operand_identifier != "RELOC")


This is basically reusing the operand_identifier as an flag, right? I suggest introducing that flag instead.

slavek-kucera · 2021-04-21T08:48:58Z

parser_library/test/diagnostics_check_test.cpp

@@ -36,7 +36,7 @@ TEST(diagnostics, overall_correctness)

    ASSERT_EQ(a.parser().getNumberOfSyntaxErrors(), (size_t)0);

-    ASSERT_EQ(dynamic_cast<hlasm_plugin::parser_library::diagnosable*>(&a)->diags().size(), (size_t)0);
+    ASSERT_EQ(dynamic_cast<hlasm_plugin::parser_library::diagnosable*>(&a)->diags().size(), (size_t)1);


Suggested change

ASSERT_EQ(dynamic_cast<hlasm_plugin::parser_library::diagnosable*>(&a)->diags().size(), (size_t)1);

ASSERT_EQ(dynamic_cast<hlasm_plugin::parser_library::diagnosable*>(&a)->diags().size(), (size_t)0);

I suggest fixing the source instead.

slavek-kucera · 2021-04-21T08:50:14Z

parser_library/test/diagnostics_check_test.cpp

@@ -236,7 +236,7 @@ TEST(diagnostics, mnemonics)

    ASSERT_EQ(a.parser().getNumberOfSyntaxErrors(), (size_t)0);

-    ASSERT_EQ(dynamic_cast<hlasm_plugin::parser_library::diagnosable*>(&a)->diags().size(), (size_t)0);
+    ASSERT_EQ(dynamic_cast<hlasm_plugin::parser_library::diagnosable*>(&a)->diags().size(), (size_t)20);


Suggested change

ASSERT_EQ(dynamic_cast<hlasm_plugin::parser_library::diagnosable*>(&a)->diags().size(), (size_t)20);

ASSERT_EQ(dynamic_cast<hlasm_plugin::parser_library::diagnosable*>(&a)->diags().size(), (size_t)0);

The same here, we should be able to create jump targets that are far enough.

parser_library/src/checking/instr_operand.cpp

michalbali256 · 2021-04-21T09:30:55Z

parser_library/src/semantics/operand_impls.cpp

+    else if (res.value_kind() == context::symbol_value_kind::RELOC && type_hint
+        && *type_hint == checking::machine_operand_type::RELOC_IMM)
+    {
+        return std::make_unique<checking::one_operand>("RELOC", res.get_reloc().offset());


This code would accept instructions like:

RELOC LR 1,1 EXRL 1,RELOC+5 EXRL 1,RELOC+RELOC

The second one is definitely invalid, I am not sure about the first one.
We should generate this special operand only when the relocatable symbol is not complex (res.get_reloc().is_simple()) and maybe even just when it is just a simple symbol (not sure about the latter).

the exrl takes the difference in halfwords (multiples of 2 bytes), so reloc+5 cannot be represented at all.

OK, but RELOC+4 would be fine?

yes, reloc+4 is ok.

Length being multiple of 2 bytes holds true only for exrl instruction or for all instructions with relative addressing?

as far as I know, it's for all reladdr operands.

Signed-off-by: Sweta Shah <swetas04@GMAIL.COM>

parser_library/src/diagnostic.cpp

slavek-kucera · 2021-05-13T18:38:46Z

parser_library/src/expressions/mach_operator.h

+        if (lc.get_reloc().offset() > symb.get_reloc().offset())
+            return (left_->evaluate(info) - right_->evaluate(info));
+        else
+            return (right_->evaluate(info) - left_->evaluate(info));


I don't understand this part... relative address may very well be positive or negative.
I'd expect simply target-location.

Okay, thanks for pointing out. I misunderstood it as difference should be always positive value, will change it to target - location.

Co-authored-by: slavek-kucera <53339291+slavek-kucera@users.noreply.github.com>

…bols

…ithub.com/SWETAS04/che-che4z-lsp-for-hlasm into Fix_format_to_support_relocatable_symbols

Signed-off-by: Sweta Shah <swetas04@GMAIL.COM>

slavek-kucera · 2021-05-24T10:29:23Z

parser_library/src/parsing/parser_impl.cpp

    {
        // replaced operands are skipped being immediate values
-        if (replaced.size() != 0 && replaced.at(position).first == position)
+        if (!replaced.empty() && replaced.at(position).first == position)


I think this is wrong... The replaced vector contains index, value pairs, so if the replacement occurs at position 3, it contains a single pair 3,value, so .at(3) will throw.
Also there may be multiple replacements in a row, so this should probably be some kind of loop.

@michalbali256 - do you concur?

slavek-kucera · 2021-05-24T10:40:57Z

parser_library/src/parsing/parser_impl.cpp

+    if (auto mnem_tmp = context::instruction::mnemonic_codes.find(*instruction);
+        mnem_tmp != context::instruction::mnemonic_codes.end())
    {
        instruction_name = context::instruction::mnemonic_codes.at(*instruction).instruction->instr_name;
        replaced = context::instruction::mnemonic_codes.at(*instruction).replaced;
    }


This could be simplified a bit to something like

Suggested change

if (auto mnem_tmp = context::instruction::mnemonic_codes.find(*instruction);

mnem_tmp != context::instruction::mnemonic_codes.end())

{

instruction_name = context::instruction::mnemonic_codes.at(*instruction).instruction->instr_name;

replaced = context::instruction::mnemonic_codes.at(*instruction).replaced;

}

const machine_instruction* instr;

const std::vector<std::pair<size_t, size_t>>* replaced = nullptr;

if (auto mnem_tmp = context::instruction::mnemonic_codes.find(*instruction);

mnem_tmp != context::instruction::mnemonic_codes.end())

{

instr = mnem_tmp->instruction;

replaced = &mnem_tmp->replaced;

}

else

{

instr = &context::instruction::machine_instructions.at(*instruction);

}

but it needs to be properly incorporated into the rest of the code

…ithub.com/SWETAS04/che-che4z-lsp-for-hlasm into Fix_format_to_support_relocatable_symbols

…p-for-hlasm into Fix_format_to_support_relocatable_symbols

slavek-kucera · 2021-06-02T10:07:38Z

parser_library/src/parsing/parser_impl.cpp

 {
+    if (instruction->empty())


when does this happen?

There is a test "parse_bad_model" where it occurs

…p-for-hlasm into Fix_format_to_support_relocatable_symbols

slavek-kucera

👍

parser_library/src/diagnostic.cpp

michalbali256 · 2021-06-09T12:10:33Z

parser_library/src/diagnostic.cpp

+{
+    return diagnostic_op(diagnostic_severity::error,
+        "ME003",
+        "Relative Immediate Operand value must be signed number of halfwords.",


This message implies to me, that the user should write a number as the argument.

Suggested change

"Relative Immediate Operand value must be signed number of halfwords.",

"Relative immediate operand must evaluate into a number of halfwords (must result in even number of bytes)",

maybe just Relative immediate operand must evaluate into an even offset?

michalbali256 · 2021-06-09T12:53:09Z

parser_library/src/semantics/operand_impls.cpp

+            checking::address_state::UNRES, 0, 0, 0, checking::operand_state::ONE_OP);
+    }
+}
+std::unique_ptr<checking::operand> check_operand(


could you rename this method? Now, it is not clear what is the difference between methods check_operand and make_check_operand

Co-authored-by: Michal Bali <38988507+michalbali256@users.noreply.github.com>

…ithub.com/SWETAS04/che-che4z-lsp-for-hlasm into Fix_format_to_support_relocatable_symbols

sonarqubecloud · 2021-06-09T14:33:41Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

96.3% Coverage
0.0% Duplication

github-actions · 2021-08-13T12:37:02Z

🎉 This PR is included in version 0.14.0-beta.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

github-actions · 2021-08-18T09:14:13Z

🎉 This PR is included in version 0.14.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

SWETAS04 and others added 5 commits March 24, 2021 09:01

Commit

4546f99

Signed-off-by: Sweta Shah <swetas04@gmail.com>

Sync with development

bd15a10

Draft

6d141b8

Signed-off-by: Sweta Shah <swetas04@GMAIL.COM>

Instructions affected by this bug added

825794b

Signed-off-by: Sweta Shah <swetas04@GMAIL.COM>

fix

e7e122f

Signed-off-by: Sweta Shah <swetas04@GMAIL.COM>

SWETAS04 marked this pull request as draft April 19, 2021 14:23

SWETAS04 added 6 commits April 19, 2021 16:52

Merge branch 'development' into Fix_format_to_support_relocatable_sym…

c30e506

…bols

Cleanup

b13fd23

Signed-off-by: Sweta Shah <swetas04@GMAIL.COM>

format

d58536d

Signed-off-by: Sweta Shah <swetas04@GMAIL.COM>

Sonar scan

4c073c0

Signed-off-by: Sweta Shah <swetas04@GMAIL.COM>

..

7c7cb03

Signed-off-by: Sweta Shah <swetas04@GMAIL.COM>

Cleanup

d8b7931

Signed-off-by: Sweta Shah <swetas04@GMAIL.COM>

SWETAS04 marked this pull request as ready for review April 21, 2021 06:56

slavek-kucera requested changes Apr 21, 2021

View reviewed changes

michalbali256 reviewed Apr 21, 2021

View reviewed changes

parser_library/src/checking/instr_operand.cpp Show resolved Hide resolved

michalbali256 reviewed Apr 21, 2021

View reviewed changes

SWETAS04 added 3 commits May 13, 2021 16:54

Requested Changes

d3b6f26

Signed-off-by: Sweta Shah <swetas04@GMAIL.COM>

Test failing

2ae7f02

Signed-off-by: Sweta Shah <swetas04@GMAIL.COM>

cleanup

8c7aa06

slavek-kucera reviewed May 13, 2021

View reviewed changes

parser_library/src/diagnostic.cpp Outdated Show resolved Hide resolved

slavek-kucera reviewed May 13, 2021

View reviewed changes

SWETAS04 and others added 9 commits May 14, 2021 10:40

Update parser_library/src/diagnostic.cpp

630575c

Co-authored-by: slavek-kucera <53339291+slavek-kucera@users.noreply.github.com>

Merge branch 'development' into Fix_format_to_support_relocatable_sym…

9792622

…bols

mnemonics not working fix

b14b8e2

Merge branch 'Fix_format_to_support_relocatable_symbols' of https://g…

88bc54d

…ithub.com/SWETAS04/che-che4z-lsp-for-hlasm into Fix_format_to_support_relocatable_symbols

format

28aef6b

Signed-off-by: Sweta Shah <swetas04@GMAIL.COM>

Conflicts causing build failure

7877561

Signed-off-by: Sweta Shah <swetas04@GMAIL.COM>

Cleanup

330d4e5

length of 2 bytes error check

39cfb16

Signed-off-by: Sweta Shah <swetas04@GMAIL.COM>

format

cdfbf7e

slavek-kucera reviewed May 24, 2021

View reviewed changes

SWETAS04 added 7 commits May 24, 2021 16:21

Position for replaced index not found correctly fix

3999b01

Delete o

f1d4749

code smell

0042876

Merge branch 'Fix_format_to_support_relocatable_symbols' of https://g…

b034d5b

…ithub.com/SWETAS04/che-che4z-lsp-for-hlasm into Fix_format_to_support_relocatable_symbols

...

f5ad18e

Merge branch 'development' of https://github.com/eclipse/che-che4z-ls…

f978b6c

…p-for-hlasm into Fix_format_to_support_relocatable_symbols

Fix for substitution operands and missing test

5be1b3b

slavek-kucera reviewed Jun 2, 2021

View reviewed changes

SWETAS04 added 5 commits June 2, 2021 12:19

Update low_language_processor.cpp

2f36a6d

Cleanup

0f56a6f

Macro operands not transformed for RI operands

92410cf

..

4c80e8f

Merge branch 'development' of https://github.com/eclipse/che-che4z-ls…

75c2054

…p-for-hlasm into Fix_format_to_support_relocatable_symbols

SWETAS04 requested a review from slavek-kucera June 9, 2021 10:57

Test for RI operands called through macro

36c86b3

slavek-kucera approved these changes Jun 9, 2021

View reviewed changes

michalbali256 reviewed Jun 9, 2021

View reviewed changes

SWETAS04 and others added 3 commits June 9, 2021 15:42

Update parser_library/src/diagnostic.cpp

0e30cb7

Co-authored-by: Michal Bali <38988507+michalbali256@users.noreply.github.com>

Requested Changes

980bcc5

Merge branch 'Fix_format_to_support_relocatable_symbols' of https://g…

d8a18f2

…ithub.com/SWETAS04/che-che4z-lsp-for-hlasm into Fix_format_to_support_relocatable_symbols

michalbali256 approved these changes Jun 9, 2021

View reviewed changes

michalbali256 merged commit e097903 into eclipse-che4z:development Jun 9, 2021

slavek-kucera linked an issue Jul 7, 2021 that may be closed by this pull request

EXRL instruction flagged as error by plugin #57

Closed

github-actions bot added the released on @release-0.14.0 label Aug 13, 2021

github-actions bot added the released label Aug 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: EXLR flagged as error by plugin #121

fix: EXLR flagged as error by plugin #121

SWETAS04 commented Apr 19, 2021

slavek-kucera Apr 21, 2021

slavek-kucera Apr 21, 2021

slavek-kucera Apr 21, 2021

slavek-kucera Apr 21, 2021

slavek-kucera Apr 21, 2021

slavek-kucera Apr 21, 2021

slavek-kucera Apr 21, 2021

michalbali256 Apr 21, 2021

slavek-kucera Apr 21, 2021

michalbali256 Apr 21, 2021

slavek-kucera Apr 21, 2021

SWETAS04 Apr 21, 2021

slavek-kucera Apr 21, 2021

slavek-kucera May 13, 2021

SWETAS04 May 14, 2021

slavek-kucera May 24, 2021

slavek-kucera May 24, 2021

slavek-kucera Jun 2, 2021

SWETAS04 Jun 2, 2021

slavek-kucera left a comment

michalbali256 Jun 9, 2021

slavek-kucera Jun 9, 2021

michalbali256 Jun 9, 2021

sonarqubecloud bot commented Jun 9, 2021

github-actions bot commented Aug 13, 2021

github-actions bot commented Aug 18, 2021

		@@ -457,20 +458,20 @@ struct diagnostic_op

		static diagnostic_op error_M112(const std::string& instr_name, const range& range);

		static diagnostic_op error_M113(const std::string& instr_name, const range& range);

	ASSERT_EQ(dynamic_cast<hlasm_plugin::parser_library::diagnosable*>(&a)->diags().size(), (size_t)1);
	ASSERT_EQ(dynamic_cast<hlasm_plugin::parser_library::diagnosable*>(&a)->diags().size(), (size_t)0);

	"Relative Immediate Operand value must be signed number of halfwords.",
	"Relative immediate operand must evaluate into a number of halfwords (must result in even number of bytes)",

fix: EXLR flagged as error by plugin #121

fix: EXLR flagged as error by plugin #121

Conversation

SWETAS04 commented Apr 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slavek-kucera left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jun 9, 2021

github-actions bot commented Aug 13, 2021

github-actions bot commented Aug 18, 2021